Skip to content

AlignmentMatrixControl: simplify styles and markup#64827

Merged
ciampo merged 10 commits into
trunkfrom
feat/alignment-matrix-control/simplify
Aug 27, 2024
Merged

AlignmentMatrixControl: simplify styles and markup#64827
ciampo merged 10 commits into
trunkfrom
feat/alignment-matrix-control/simplify

Conversation

@ciampo

@ciampo ciampo commented Aug 27, 2024

Copy link
Copy Markdown
Contributor

What?

In preparation for #64723

Simplify AlignmentMatrixControl styles, rewrite AlignmentMatrixControl.Icon using SVG, add better focus styles and support for Windows High Contrast mode.

Why?

While working on #64723, I realized that AlignmentMatrixControl and AlignmentMatrixControl.Icon have some unnecessarily complex code, which made the refactor in #64723 harder than it should have been.

How?

  • AlignmentMatrixControl:
    • Instead of using JS logic, the active Composite item is styled based on the data-active-item attribute;
    • Added a focus ring only when using the keyboard;
    • The component is now fully compatible with Windows High Contrast mode (see attachments below);
    • Stopped using deprecated COLORS variables;
  • AlignmentMatrixControl.Icon:
    • Rewrote completely using SVG — it is an icon after all, and I didn't see valid reasons for using non-svg markup. Using SVG also means that all additional custom styles are unnecessary.
    • The component is now fully compatible with Windows High Contrast mode (see attachments below)
    • The size prop on AlignmentMatrixControl.Icon is deprecated, in favor of using width and height. This is mostly there to reflect that this icon is supposed to be used in combination with the <Icon /> component (as shown in the Storybook examples and done in the editor), which accepts a size prop itself, and internally sets the width and height props on the SVG.
    • Previously, the scaling of AlignmentMatrixControl.Icon was done by applyingtransform: scale() via CSS — which meant that the footprint of the icon in the layout flow would stay the same (24px) regardless of the size. With the changes in this PR, the AlignmentMatrixControl.Icon correctly occupies an amount of space equal to its width and height.
    • The AlignmentMatrixControl.Icon now accepts SVG-related HTML attributes, while previously it accepted HTML div ones (this is technically a breaking change, although I don't see how consumers would be affected)

Testing Instructions

  • Make sure unit tests pass
  • Check the components in Storybook and make sure they look and behave like on trunk
    • Check that the version from this PR shows a focus ring around the selected AlignmentMatrixControl item when using the keyboard (and not when using mouse);
    • Check that both AlignmentMatrixControl and AlignmentMatrixControl.Icon can be perceived and interacted with in Windows High Contrast mode (this can also be testing by emulating the forced-colors media query)
  • In the editor, add a Cover block and add an image:
    • In the block toolbar, notice the AlignmentMatrixControl.Icon being used in a toolbar item. Clicking it will open a popover containing a AlignmentMatrixControl component;
    • Interact with both components, make sure they work as expected
    • Follow the same instructions shared above about testing focus rings and Windows High Contrast mode

Screenshots or screencast

Before (trunk) After (this PR)
amc.-.sb.-.trunk.mp4
amc.-.sb.-.pr.mp4
Screenshot 2024-08-27 at 15 23 06 Screenshot 2024-08-27 at 15 24 29
Screenshot 2024-08-27 at 15 24 03 Screenshot 2024-08-27 at 15 25 12
amc.-.editor.-.trunk.mp4
amc.-.editor.-.pr.mp4

@ciampo ciampo force-pushed the feat/alignment-matrix-control/simplify branch from 994bf2d to 59538e2 Compare August 27, 2024 12:56
@ciampo ciampo requested review from a team and t-hamano August 27, 2024 13:34
@ciampo ciampo self-assigned this Aug 27, 2024
@ciampo ciampo added [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Aug 27, 2024
@ciampo ciampo marked this pull request as ready for review August 27, 2024 13:35
@ciampo ciampo requested a review from ajitbohra as a code owner August 27, 2024 13:35
@github-actions

github-actions Bot commented Aug 27, 2024

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@mirka mirka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice cleanup, and great bug fix with the Windows High Contrast mode!

Comment thread packages/components/src/alignment-matrix-control/icon.tsx Outdated
Comment thread packages/components/src/alignment-matrix-control/types.ts
@mirka mirka changed the title AlignmentMatrixControl: simply styles and markup AlignmentMatrixControl: simplify styles and markup Aug 27, 2024
@ciampo ciampo force-pushed the feat/alignment-matrix-control/simplify branch from 6faf7ee to 765ab40 Compare August 27, 2024 18:40
@ciampo ciampo enabled auto-merge (squash) August 27, 2024 18:40
@ciampo ciampo disabled auto-merge August 27, 2024 18:57
@ciampo ciampo enabled auto-merge (squash) August 27, 2024 19:03
@ciampo ciampo merged commit 5773ae6 into trunk Aug 27, 2024
@ciampo ciampo deleted the feat/alignment-matrix-control/simplify branch August 27, 2024 19:36
@github-actions github-actions Bot added this to the Gutenberg 19.2 milestone Aug 27, 2024
@t-hamano

Copy link
Copy Markdown
Contributor

Sorry for the late reply. Just wanted to let you know that it works as expected in Windows high contrast mode too 👍

a647ab134603f1bd2c17e1158671d1de.mp4
9fbfaae00bd0f7427db6f19372a29360.mp4

bph pushed a commit to bph/gutenberg that referenced this pull request Aug 31, 2024
* Simplify icon active cell computation

* AlignmentMatrixControl: simplify styles

* AlignmentMatrixControl.Icon: rewrite using SVG

* Do not use `size` prop on AlignmentMatrixControl.Icon in Storybook

* Cleanup

* CHANGELOG

* Do not type height and width explicitly

* Fix typo

* Inline computedWidth and computedHeight

* Restore getAlignmentIndex function, since it normalizes values internally

---

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants