Bug 233055 – Web Inspector: Add a swatch for justify-content, justify-items, and justify-self

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

.

[Radar WebKit Bug Importer]

<rdar://problem/85613257>

[Nikita Vasilyev]

Created attachment 446321 [details]
WIP (not for review)

Experimenting with reusing current align-* icons and rotating them with CSS.

[Nikita Vasilyev]

Created attachment 446790 [details]
Patch

This should merge on top of Bug 233054 - Web Inspector: Add a swatch for align-items and align-self

[Nikita Vasilyev]

Created attachment 446793 [details]
Patch

Rebaseline.

[Nikita Vasilyev]

Created attachment 446797 [details]
[Video] With patch applied

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #5)
> Created attachment 446797 [details]
> [Video] With patch applied

justify-content icons look like a barcode, which isn't great. I'm going to address this in Bug 234036 - Web Inspector: Revamp visual design of Alignment editor.

[Patrick Angle]

Comment on attachment 446793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446793&action=review

> Source/WebInspectorUI/ChangeLog:16
> +        The newly added swatches reuse the existing `align-content` and `align-items` icons,
> +        and rotate them -90 degrees.

Can you add a note here that the icons need to be rotated -90deg is because while the `align-*` properties define alignment in the block-direction the `justify-*` properties define alignment in the inline-direction?

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:57
> +        switch (type) {
> +        case WI.AlignmentData.Type.JustifyContent:
> +        case WI.AlignmentData.Type.JustifyItems:
> +        case WI.AlignmentData.Type.JustifySelf:
> +            return true;
> +        }
> +        return false;

Preferably the switch statement would be exhaustive to make sure any new `WI.AlignmentData.Type` is accounted for, with an assertion afterwards that the `type` was handled (followed by the fail-safe `return false;`)

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
> +            if (WI.AlignmentEditor.shouldRotateGlyph(value.type))
> +                this._swatchInnerElement.style.rotate = "-90deg";

Should we set the rotation back to a normal value if we shouldn't rotate the glyph?

[Devin Rousso]

Comment on attachment 446793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446793&action=review

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:107
> +                if (shouldRotate)
> +                    glyphElement.querySelector("svg").style.rotate = "-90deg";

ditto (Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209)

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
>> +                this._swatchInnerElement.style.rotate = "-90deg";
> 
> Should we set the rotation back to a normal value if we shouldn't rotate the glyph?

+1

Also, I think this would be better as a CSS class so that you can just
```
    this._swatchInnerElement.classList.toggle("rotate-left", WI.AlignmentEditor.shouldRotateGlyph(value.type));
```

[Nikita Vasilyev]

Comment on attachment 446793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446793&action=review

>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:107
>> +                    glyphElement.querySelector("svg").style.rotate = "-90deg";
> 
> ditto (Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209)

This doesn't need to do the same because the glyphs are created from scratch here (see this._element.removeChildren() at line 90).

>>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
>>> +                this._swatchInnerElement.style.rotate = "-90deg";
>> 
>> Should we set the rotation back to a normal value if we shouldn't rotate the glyph?
> 
> +1
> 
> Also, I think this would be better as a CSS class so that you can just
> ```
>     this._swatchInnerElement.classList.toggle("rotate-left", WI.AlignmentEditor.shouldRotateGlyph(value.type));
> ```

Patrick: yes, we should!

[Patrick Angle]

Comment on attachment 446793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446793&action=review

>>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:107
>>> +                    glyphElement.querySelector("svg").style.rotate = "-90deg";
>> 
>> ditto (Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209)
> 
> This doesn't need to do the same because the glyphs are created from scratch here (see this._element.removeChildren() at line 90).

I would still suggest switching to the CSS class one-liner Devin suggested regardless for consistency's sake.

[Nikita Vasilyev]

Created attachment 446821 [details]
Patch

[Patrick Angle]

Comment on attachment 446821 [details]
Patch

Looks good to me, but I'd like to see a patch that actually applies to the tree 😅 - I think this one needs rebased on the other swatch work you landed earlier today.

[Nikita Vasilyev]

Created attachment 446828 [details]
Patch

Rebaselined.

[Patrick Angle]

Comment on attachment 446828 [details]
Patch

r=me Nice work.

[Devin Rousso]

Comment on attachment 446828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446828&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.css:58
> +.alignment-editor .glyph.rotate-left > svg {

Is the `> svg` necessary?

[Nikita Vasilyev]

Comment on attachment 446828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446828&action=review

>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.css:58
>> +.alignment-editor .glyph.rotate-left > svg {
> 
> Is the `> svg` necessary?

Yes, because of the way I currently set left/right the borders on `.glyph` (see line 37 and 50).

[EWS]

Committed r286885 (245114@main): <https://commits.webkit.org/245114@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446828 [details].