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

Upgrade Reakit to stable v1.0.0 #22352

Merged
merged 2 commits into from
May 14, 2020
Merged

Upgrade Reakit to stable v1.0.0 #22352

merged 2 commits into from
May 14, 2020

Conversation

diegohaz
Copy link
Member

Description

This PR upgrades the reakit dependency on @wordpress/components to the stable v1.0.0.

Two things were fixed that are relevant to us:

  1. onMouseDown on ToolbarItems no longer prevents drag events from happening.
  2. CompositeItem is now focused on VoiceOver clicks (Control+Option+Space), so we don't need this piece of code on AlignmentMatrixControl anymore.

How has this been tested?

  1. Check toolbars are working properly.
  2. Check AlignmentMatrixControl introduced on Cover: Customizing Alignment of inner content #21091.

@diegohaz diegohaz requested review from ItsJonQ and gziolo May 14, 2020 13:23
@diegohaz diegohaz self-assigned this May 14, 2020
@diegohaz diegohaz added [Package] Components /packages/components [Block] Cover Affects the Cover Block - used to display content laid over a background image labels May 14, 2020
@github-actions
Copy link

github-actions bot commented May 14, 2020

Size Change: -1.1 kB (0%)

Total Size: 832 kB

Filename Size Change
build/block-directory/index.js 6.59 kB -1 B
build/block-editor/index.js 104 kB -329 B (0%)
build/block-library/index.js 118 kB -2 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB +1 B
build/blocks/index.js 48.1 kB -1 B
build/components/index.js 182 kB -39 B (0%)
build/compose/index.js 6.67 kB -3 B (0%)
build/core-data/index.js 11.4 kB -9 B (0%)
build/data-controls/index.js 1.29 kB -1 B
build/data/index.js 8.43 kB -5 B (0%)
build/date/index.js 5.47 kB -4 B (0%)
build/dom-ready/index.js 569 B +1 B
build/dom/index.js 3.1 kB +1 B
build/edit-navigation/index.js 5.6 kB -2 B (0%)
build/edit-post/index.js 28.1 kB -74 B (0%)
build/edit-site/index.js 12 kB +3 B (0%)
build/edit-widgets/index.js 7.86 kB -605 B (7%)
build/editor/index.js 44.3 kB -20 B (0%)
build/format-library/index.js 7.63 kB -2 B (0%)
build/hooks/index.js 2.13 kB -1 B
build/i18n/index.js 3.56 kB -1 B
build/is-shallow-equal/index.js 711 B -1 B
build/keyboard-shortcuts/index.js 2.51 kB +1 B
build/keycodes/index.js 1.94 kB -1 B
build/media-utils/index.js 5.29 kB +2 B (0%)
build/nux/index.js 3.4 kB +2 B (0%)
build/plugins/index.js 2.56 kB -1 B
build/primitives/index.js 1.5 kB +1 B
build/redux-routine/index.js 2.85 kB +3 B (0%)
build/rich-text/index.js 14.8 kB -2 B (0%)
build/server-side-render/index.js 2.67 kB -6 B (0%)
build/url/index.js 4.02 kB -1 B
build/wordcount/index.js 1.17 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 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-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 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/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.49 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-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/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/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 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/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/priority-queue/index.js 789 B 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B

compressed-size-action

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code looks good, it's great to see stable version integrated :)

I guess it's best to leave it to @ItsJonQ to ensure that updated components work as expected. I can try to validate myself tomorrow if he won't be able to take care of it.

@diegohaz
Copy link
Member Author

Fixed unit tests. We may want to use @testing-library/user-event in the future.

@ItsJonQ Also, I remember you tried to use aria-activedescendant on AlignmentMatrixControl in the first iterations. If it's still needed, it should be working now (by passing unstable_virtual: true to useCompositeState).

@gziolo
Copy link
Member

gziolo commented May 14, 2020

Fixed unit tests. We may want to use @testing-library/user-event in the future.

I have too many things going on, but I would be interested in doing an audit of existing RTL tests and updating them according to the guidelines shared by @kentcdodds in https://kentcdodds.com/blog/common-mistakes-with-react-testing-library. This way we will be able to reference this article in the testing guidelines and call it a day. I think it also means we should include the library you mentioned :)

Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

@diegohaz Looks good to me! 🚀
Tested it locally with Toolbars and the Alignment controls, and it works as expected

Also, I remember you tried to use aria-activedescendant on AlignmentMatrixControl in the first iterations. If it's still needed, it should be working now (by passing unstable_virtual: true to useCompositeState).

I briefly tried passing the unstable_virtual prop into the useCompositeState. I didn't notice a difference, which I suppose is a good thing. I remember it not working previously.

We can test it some more and update it after this is merged 👏

@diegohaz diegohaz merged commit 196d8ce into master May 14, 2020
@diegohaz diegohaz deleted the add/upgrade-reakit branch May 14, 2020 17:07
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 14, 2020
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 [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants