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

IconButton: refs don't work #12615

Closed
afercia opened this issue Dec 5, 2018 · 4 comments
Closed

IconButton: refs don't work #12615

afercia opened this issue Dec 5, 2018 · 4 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Dec 5, 2018

While refs on the Button component work, they don't on the IconButton component.

IconButton is a class that wraps Button and doesn't pass any ref to Button, which in turn uses forwardRef().

A similar case has been addressed for the PanelBody component, see:

const forwardedPanelBody = ( props, ref ) => {
return <PanelBody { ...props } forwardedRef={ ref } />;
};
forwardedPanelBody.displayName = 'PanelBody';
export default forwardRef( forwardedPanelBody );

Not saying this is necessarily the best option, but being able to get a ref to IconButton would be nice. Worth reminding refs help in managing focus when necessary, so they are important for accessibility.

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Component] Button labels Dec 5, 2018
@afercia
Copy link
Contributor Author

afercia commented Feb 27, 2019

Played a bit with this:

  • the forwardRef approach works but then fixing the tests is a bit challenging (at least for me) as some of them depends on the component's name
  • alternatively, I think a simple innerRef prop passed down to Button would work?

Seems to me the first approach is preferable but this issue needs someone with expertise in building tests 🙂

@aduth
Copy link
Member

aduth commented Feb 27, 2019

Seems to me the first approach is preferable but this issue needs someone with expertise in building tests 🙂

I'd agree with this.

Looking at the following, I think there was some hacking done to make Button and its forwardRef usage play nicely with tests / Enzyme:

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/button/__mocks__/index.js

Since that time, Enzyme has updated to provide more up-to-date adapters. Updating those may resolve some issue there to test the component. It's been on my task list to take a look at this.

In parallel, it might make sense to push up what you have (even failing) as a pull request, and collaborate on resolving the test failures.

@aduth
Copy link
Member

aduth commented Feb 27, 2019

Since that time, Enzyme has updated to provide more up-to-date adapters. Updating those may resolve some issue there to test the component. It's been on my task list to take a look at this.

See: #11214, #14156

@afercia
Copy link
Contributor Author

afercia commented Feb 27, 2019

Yep, spent some (too much) time on this and then saw the button mock, researched what the real problem was, and finally found the Enzyme issue with forwardRef. Some of the tests can be worked around, but I guess the IconButton tests will need the updated Enzyme stuff. Will push what I have, thanks!

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). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

2 participants