Bug 233055 - Web Inspector: Add a swatch for justify-content, justify-items, and justify-self
Summary: Web Inspector: Add a swatch for justify-content, justify-items, and justify-self
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 233054
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-12 11:36 PST by Nikita Vasilyev
Modified: 2021-12-10 16:42 PST (History)
4 users (show)

See Also:


Attachments
WIP (not for review) (3.14 KB, patch)
2021-12-08 00:10 PST, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2021-12-10 13:05 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (5.80 KB, patch)
2021-12-10 13:18 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Video] With patch applied (6.00 MB, video/quicktime)
2021-12-10 13:22 PST, Nikita Vasilyev
no flags Details
Patch (7.13 KB, patch)
2021-12-10 14:51 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2021-12-10 15:22 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2021-11-12 11:36:32 PST
.
Comment 1 Radar WebKit Bug Importer 2021-11-19 11:37:39 PST
<rdar://problem/85613257>
Comment 2 Nikita Vasilyev 2021-12-08 00:10:59 PST
Created attachment 446321 [details]
WIP (not for review)

Experimenting with reusing current align-* icons and rotating them with CSS.
Comment 3 Nikita Vasilyev 2021-12-10 13:05:31 PST
Created attachment 446790 [details]
Patch

This should merge on top of Bug 233054 - Web Inspector: Add a swatch for align-items and align-self
Comment 4 Nikita Vasilyev 2021-12-10 13:18:13 PST
Created attachment 446793 [details]
Patch

Rebaseline.
Comment 5 Nikita Vasilyev 2021-12-10 13:22:46 PST
Created attachment 446797 [details]
[Video] With patch applied
Comment 6 Nikita Vasilyev 2021-12-10 13:27:52 PST
(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.
Comment 7 Patrick Angle 2021-12-10 14:16:30 PST
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?
Comment 8 Devin Rousso 2021-12-10 14:34:57 PST
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));
```
Comment 9 Nikita Vasilyev 2021-12-10 14:42:25 PST
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!
Comment 10 Patrick Angle 2021-12-10 14:46:15 PST
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.
Comment 11 Nikita Vasilyev 2021-12-10 14:51:22 PST
Created attachment 446821 [details]
Patch
Comment 12 Patrick Angle 2021-12-10 15:13:22 PST
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.
Comment 13 Nikita Vasilyev 2021-12-10 15:22:55 PST
Created attachment 446828 [details]
Patch

Rebaselined.
Comment 14 Patrick Angle 2021-12-10 15:37:27 PST
Comment on attachment 446828 [details]
Patch

r=me Nice work.
Comment 15 Devin Rousso 2021-12-10 15:39:12 PST
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?
Comment 16 Nikita Vasilyev 2021-12-10 15:42:21 PST
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).
Comment 17 EWS 2021-12-10 16:42:12 PST
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].