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

fix: Stop warning when using invalid composite roles on Composite #784

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

diegohaz
Copy link
Member

This PR removes the warning that Composite shows when the developer doesn't pass any role prop or pass an invalid composite role prop to the Composite component.

// Will not warn anymore
<Composite />

The reason is that there are valid use cases for omitting the role prop. For example, we could create multiple connected listbox elements to implement the ARIA 1.2 listbox group pattern for screen readers that still don't support it, like VoiceOver. This use case is better explained on WordPress/gutenberg#24975 (comment).

With the Reakit Composite module, the implementation would be something like this, where the listbox role is not passed to the Composite component.

const composite = useCompositeState();
<Composite {...composite}>
  <div role="listbox" aria-label="Group 1">
    <CompositeItem {...composite} role="option">Item</CompositeItem>
    <CompositeItem {...composite} role="option">Item</CompositeItem>
    <CompositeItem {...composite} role="option">Item</CompositeItem>
  </div>
  <div role="listbox" aria-label="Group 2">
    <CompositeItem {...composite} role="option">Item</CompositeItem>
    <CompositeItem {...composite} role="option">Item</CompositeItem>
    <CompositeItem {...composite} role="option">Item</CompositeItem>
  </div>
  <div role="listbox" aria-label="Group 3">
    <CompositeItem {...composite} role="option">Item</CompositeItem>
    <CompositeItem {...composite} role="option">Item</CompositeItem>
    <CompositeItem {...composite} role="option">Item</CompositeItem>
  </div>
</Composite>

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c73e533:

Sandbox Source
Reakit Configuration

@github-actions
Copy link
Contributor

Size Change: 0 B

Total Size: 252 kB

ℹ️ View Unchanged
Filename Size Change
packages/reakit-playground/dist/reakit-playground.min.js 186 kB 0 B
packages/reakit-system-bootstrap/dist/reakit-system-bootstrap.min.js 19.2 kB 0 B
packages/reakit-system-palette/dist/reakit-system-palette.min.js 8.86 kB 0 B
packages/reakit-system/dist/reakit-system.min.js 2.22 kB 0 B
packages/reakit-utils/dist/reakit-utils.min.js 3.24 kB 0 B
packages/reakit/dist/reakit.min.js 32.3 kB 0 B

compressed-size-action

@ariakit-bot
Copy link

Deploy preview for reakit ready!

Built with commit c73e533

https://deploy-preview-784--reakit.netlify.app

@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #784 (c73e533) into master (7a5d193) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage   95.68%   95.68%   -0.01%     
==========================================
  Files         190      190              
  Lines        2990     2987       -3     
  Branches      776      775       -1     
==========================================
- Hits         2861     2858       -3     
  Misses        129      129              
Impacted Files Coverage Δ
packages/reakit/src/Composite/Composite.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a5d193...c73e533. Read the comment docs.

@diegohaz diegohaz merged commit 9bcbfe6 into master Nov 12, 2020
@diegohaz diegohaz deleted the fix/composite-role-warning branch November 12, 2020 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants