Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cover: Customizing Alignment of inner content #21091

Merged
merged 35 commits into from May 13, 2020
Merged

Conversation

ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented Mar 23, 2020

Description

Screen Capture on 2020-03-25 at 17-26-51

This update brings the ability to adjust the vertical and horizontal alignment of content with the Cover block. The control is provided by a new (experimental) AlignmentMatrixControl component from the @wordpress/components package.

Attempts to resolve: #20836

Control

Screen Capture on 2020-03-23 at 16-52-09

The new AlignmentMatrixControl provides a "9 point" alignment interface. This UI can be controlled by clicking on any of the points. Alternatively, pressing UP, DOWN, LEFT, or RIGHT on the keyboard would change the alignment values.

Icon

76532112-70d2c080-6476-11ea-8dd0-1fedee597c2f

In addition to the control, the AlignmentMatrixControl provides a companion icon, which can be loaded into UI like the Toolbar. It accepts an alignment prop, which updates the UI to visually reflect the alignment.

The icon implementation is designed to work like an icon from @wordpress/icons

Example
import { __experimentalAlignmentMatrixControl as AlignmentMatrixControl } from "@wordpress/components";
import { Icon } from "@wordpress/icons";

const Example = () => {
  return <Icon icon={AlignmentMatrixControl.icon} alignment="bottom left" />;
};

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ItsJonQ ItsJonQ added [Package] Components /packages/components [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Mar 23, 2020
@ItsJonQ ItsJonQ self-assigned this Mar 23, 2020
@ItsJonQ ItsJonQ requested a review from mtias March 23, 2020 21:04
@github-actions
Copy link

github-actions bot commented Mar 23, 2020

Size Change: +2.51 kB (0%)

Total Size: 831 kB

Filename Size Change
build/annotations/index.js 3.62 kB -3 B (0%)
build/block-directory/index.js 6.59 kB -2 B (0%)
build/block-editor/index.js 104 kB +231 B (0%)
build/block-editor/style-rtl.css 10.8 kB +32 B (0%)
build/block-editor/style.css 10.8 kB +32 B (0%)
build/block-library/index.js 116 kB +499 B (0%)
build/block-library/style-rtl.css 7.48 kB +145 B (1%)
build/block-library/style.css 7.49 kB +143 B (1%)
build/blocks/index.js 48.1 kB +15 B (0%)
build/components/index.js 182 kB +1.35 kB (0%)
build/compose/index.js 6.68 kB +16 B (0%)
build/core-data/index.js 11.4 kB -1 B
build/data-controls/index.js 1.29 kB +2 B (0%)
build/data/index.js 8.43 kB +5 B (0%)
build/date/index.js 5.47 kB +1 B
build/edit-post/index.js 28 kB +28 B (0%)
build/edit-site/index.js 12.1 kB -3 B (0%)
build/edit-widgets/index.js 8.37 kB -3 B (0%)
build/editor/index.js 44.3 kB +6 B (0%)
build/element/index.js 4.65 kB +2 B (0%)
build/escape-html/index.js 733 B -1 B
build/format-library/index.js 7.63 kB +2 B (0%)
build/is-shallow-equal/index.js 712 B +2 B (0%)
build/nux/index.js 3.4 kB +2 B (0%)
build/plugins/index.js 2.56 kB +2 B (0%)
build/redux-routine/index.js 2.85 kB +1 B
build/rich-text/index.js 14.8 kB -3 B (0%)
build/token-list/index.js 1.28 kB +1 B
build/wordcount/index.js 1.18 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 764 B 0 B
build/block-directory/style.css 764 B 0 B
build/block-library/editor-rtl.css 7.25 kB 0 B
build/block-library/editor.css 7.25 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 5.59 kB 0 B
build/edit-navigation/style-rtl.css 618 B 0 B
build/edit-navigation/style.css 617 B 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B

compressed-size-action

@@ -0,0 +1,18 @@
# AlignmentControl
Copy link
Member

Choose a reason for hiding this comment

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

Should we call this AlignmentMatrix to better separate from BlockAlignment, etc?

Copy link
Author

Choose a reason for hiding this comment

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

We could! Although I think having Control in the component name would be helpful, as it has the value/onChange interface.

AlignmentMatrixControl? or AlignmentGridControl?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, especially if we reserve the AlignmentMatrix for the toolbar component.

@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 24, 2020

Screen Shot 2020-03-24 at 12 32 11 PM

Still very WIP! I have this so far.

In doing so... I'm realizing that it's quite tricky to adjust the size of the Dropdown Menu within this context 🤔 .

@ItsJonQ ItsJonQ force-pushed the try/cover-alignment-control branch from bd2bf93 to 710c33a Compare March 25, 2020 20:33
@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 25, 2020

Oh boy!!

Screen Capture on 2020-03-25 at 17-26-51

@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 26, 2020

An observation!

In order for non-centered alignment to look okay, I had to add extra padding around the content. That way, the text doesn't touch the edges.

@pablohoneyhoney
Copy link

Fair!

It’d be good to test it with an image if reduced within the cover area.

@mtias
Copy link
Member

mtias commented Mar 26, 2020

@ItsJonQ some default padding makes sense, indeed, but it should be just the natural padding of the "cover" block — which as part of this work we should make configurable as well.

This is looking great!

@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 27, 2020

but it should be just the natural padding of the "cover" block

Ah gotcha! Thanks for that feedback @mtias ! I'll make those adjustments now :)

@ItsJonQ ItsJonQ force-pushed the try/cover-alignment-control branch from 093de87 to 6bf9103 Compare March 27, 2020 13:33
@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 27, 2020

Screen Shot 2020-03-27 at 11 12 22 AM

Working on a11y support. After that, will add RTL support!

@ItsJonQ ItsJonQ added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Mar 27, 2020
@ItsJonQ ItsJonQ requested a review from diegohaz March 27, 2020 15:45
@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 27, 2020

a11y + RTL updates

Screen Capture on 2020-03-27 at 11-43-18

The GIF above showcases Mac OS voiceover describing the alignment in when changes are made (keyboard navigation). The control is also rendering in rtl, where the labels and values are flipped.

The goal for this control is to be able to successfully navigate the 9x9 grid with keyboard (as well as clicks).
The a11y labels should describe the position for each cell.
Lastly, the navigation + labels should flip to reflect RTL documents.

I gave it my best shot, although admittedly, I'm lacking some experience when it comes to a11y support for custom controls such as this one.

(cc'ing @diegohaz)

I would love some insight and expertise here!

I made an attempt at aria labels - I am unsure what WAI-ARIA role fits this particular 9x9 alignment UI the best.

@gziolo Also pointed out the Composite component from Reakit to me. If possible, it would be great to leverage it here 😍

Thank you all so much!

@ItsJonQ ItsJonQ added the Needs Accessibility Feedback Need input from accessibility label Mar 27, 2020
@diegohaz
Copy link
Member

Great job! It's working really well with keyboard. It can be improved for screen readers though.

I am unsure what WAI-ARIA role fits this particular 9x9 alignment UI the best.

It's definitely a grid. Listbox is unidirectional. Even though sighted users would see that it's a grid and they could guess they could use arrow up and down to navigate vertically, there's no hint for screen reader users, unless you use role="grid" (which is equivalente to <table>).

I couldn't reproduce what you show on the GIF using VoiceOver + Chrome and Safari. I can only navigate between the grid items using VO (option+ctrl) + directional keys. And it only navigates horizontally. Using up and down arrow keys (without VO key) only moves to the previous and next elements of the type I selected in the VoiceOver Rotor (by pressing up and right keys fast). For example, if it's set to navigate between links, pressing arrow down would put me in the next link. Maybe it's the MacOS version (10.15.3) or some configuration on VoiceOver?

If that's a role="grid", in combination with role="row" and role="gridcell", the screen reader will properly announce the row and column numbers and let you use VO + up or down keys to move vertically.

@gziolo Also pointed out the Composite component from Reakit to me. If possible, it would be great to leverage it here 😍

That's still experimental, but you can see many implementation examples here, including grid: https://codesandbox.io/s/composite-gcqs2

@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 31, 2020

@diegohaz Wonderful 😍 ! Thank you so much for your feedback! This is tremendously helpful!
I'll move forward with your suggestions

@ItsJonQ ItsJonQ force-pushed the try/cover-alignment-control branch from 4708549 to 98692da Compare April 1, 2020 15:45
@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 1, 2020

@diegohaz Haii! I started looking into using Composite. The API feels great :)

The only thing I'm unsure of is if I'm able to tap into an onChange callback of some kind. For example, when I'm using it to display the 9x9 grid, I need to manage state (and pass up) changes.

const composite = useCompositeState({
    ...
    onChange: handleOnChange
});

Is it possible to do something like this? If not, is there a preferred way of handling this?

Thank you! <3

@diegohaz
Copy link
Member

diegohaz commented Apr 1, 2020

@ItsJonQ Depending on what you want to do, you can simply attach an onFocus or onClick handler to the item:

<Composite>
  ...
  <CompositeItem onFocus={select} onClick={select} />
  ...
</Composite>

If it's not enough, you can track the composite.currentId state:

// sets initial currentId
const composite = useCompositeState({ currentId: "unique-id-center-center" });

// selects the currentId whenever it changes
React.useEffect(() => {
  select(composite.currentId);
}, [composite.currentId]);

<Composite>
  ...
  <CompositeItem id="unique-id-center-center" />
  ...
</Composite>

@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 1, 2020

@diegohaz Ooo! Gotcha. Thank you! I'll give that a shot!

@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 1, 2020

Update! I attempted to implement Composite into AlignmentMatrixControl.
It seemed to work okay, but it's having some issues in the editor.

I've created a separate branch + Draft PR here:
#21333

@mtias mtias force-pushed the try/cover-alignment-control branch from 98692da to b51794c Compare April 2, 2020 10:52
@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 3, 2020

Just merged in the a11y work to integrate Reakit Composite to take care of the interactions and aria semantics 🙌

(Massive props to @diegohaz !)

With that, I think this PR is ready for review

@ItsJonQ ItsJonQ force-pushed the try/cover-alignment-control branch from fd051c4 to 96674de Compare May 13, 2020 13:24
@ItsJonQ
Copy link
Author

ItsJonQ commented May 13, 2020

@mtias Thank you for reviewing! I've rebased with latest master. Will merge this up once Travis is green and happy :)

Due to lack of polyfill support (at the moment).
@ItsJonQ
Copy link
Author

ItsJonQ commented May 13, 2020

Yay! Merging! 😊

@ItsJonQ ItsJonQ merged commit 47389d5 into master May 13, 2020
@ItsJonQ ItsJonQ deleted the try/cover-alignment-control branch May 13, 2020 14:46
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 13, 2020
@mtias
Copy link
Member

mtias commented May 13, 2020

Nice work, really like this one :)

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 8, 2020

Hey

I added a background image. A title and a button below it. But I am having a hard time aligning the title and the button to the middle of the Cover Block.

Title and button have no alignment selected.

Screen Shot 2020-06-08 at 22 41 17

Aligning the title and the button to center looks like this:
(Grid is center aligned.) Here I have selected both the button and the title.

Screen Shot 2020-06-08 at 22 45 00

I am guessing a new issue is needed to address the centering of the Button.
Here is the issue: #23010

@rmorse
Copy link
Contributor

rmorse commented Dec 31, 2020

Found a little bug when opening the control, focus is triggered (and so is onChange) - the fix is here #27945

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover: internal alignment / position
8 participants