Skip to content

Conversation

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 20, 2019

  • remove redundant aria-hidden, visibility is a better alternative
  • add explicit boolean cast to aria-expanded in ExpansionPanelSummary. aria-expanded={undefined} will not appear in the DOM

Also in this PR: It's not vibility or visiblity. It's visibility.

@eps1lon eps1lon added the accessibility a11y label Feb 20, 2019
disabled={disabled}
component="div"
aria-expanded={expanded}
aria-expanded={Boolean(expanded)}
Copy link
Member

Choose a reason for hiding this comment

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

What's the need for this change?

expanded: PropTypes.bool,

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's passed as undefined it won't appear in the DOM not indicating that the element expands some content.

Copy link
Member

@oliviertassinari oliviertassinari Feb 21, 2019

Choose a reason for hiding this comment

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

In practice, it should always be defined:
https://github.com/mui-org/material-ui/blob/64013d6c817b3c9bf2bd006aee2ac3f4a948a01f/packages/material-ui/src/ExpansionPanel/ExpansionPanel.js#L134-L137

However, we can't used .isRequired because it depends on the clone element method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we arguing whether we should put this in? Why not be safe and always have it as a boolean. It's explicit, helps test. Could you elaborate what the downsides are here?

Copy link
Member

@oliviertassinari oliviertassinari Feb 21, 2019

Choose a reason for hiding this comment

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

What I'm saying is that we can be "smart" about it, "smart" in the sense that the logic assumes more context. I'm showing an alternative viable option. I'm not saying it's better. We know that in practice, the Boolean() will be equivalent to an identity function, so it's not a requirement.

It's explicit, helps test.

I believe the tests could always provide a boolean value if we want to match reality. We could also add a warning in the render method.


So now, what's best? I would go for reducing the entropy: add a runtime warning / throw if expanded is undefined. It's one less case to consider, no defensive checks etc. (It's not a really important matter, pick what you think is best)

Copy link
Member Author

@eps1lon eps1lon Feb 21, 2019

Choose a reason for hiding this comment

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

You're right. I'm removing this. I think it might be a better idea to move those controlled props into context anyway. Gets rid of cloneElement, allows a fragment as a child and we can improve the DX by setting the default value of expanded to a boolean. That's for another PR though

@eps1lon
Copy link
Member Author

eps1lon commented Feb 21, 2019

@oliviertassinari
Copy link
Member

lol…

@eps1lon eps1lon changed the title [core] Improve a11y for Collapse, ExpansionPanel, ExpansionPanelSummary and Grow [core] Improve a11y for Collapse, ExpansionPanel and Grow Feb 21, 2019
@eps1lon eps1lon merged commit c3a4feb into mui:next Feb 21, 2019
@eps1lon eps1lon deleted the feat/ExpansionPanelSummary/a11y-controls branch February 21, 2019 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants