Summary: | Web Inspector: Add a swatch for justify-content, justify-items, and justify-self | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | hi, inspector-bugzilla-changes, pangle, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 233054 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2021-11-12 11:36:32 PST
Created attachment 446321 [details]
WIP (not for review)
Experimenting with reusing current align-* icons and rotating them with CSS.
Created attachment 446790 [details] Patch This should merge on top of Bug 233054 - Web Inspector: Add a swatch for align-items and align-self Created attachment 446793 [details]
Patch
Rebaseline.
Created attachment 446797 [details]
[Video] With patch applied
(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 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 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 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 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. Created attachment 446821 [details]
Patch
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.
Created attachment 446828 [details]
Patch
Rebaselined.
Comment on attachment 446828 [details]
Patch
r=me Nice work.
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 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). 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]. |