-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
remove defaultProp from required property in components/modal #16074
remove defaultProp from required property in components/modal #16074
Conversation
I'm wondering whether we should update docs to reflect that it is optional instead. It seems to be safer. |
That works too! Won't hurt my feelings to close this PR if that's the preferred route 😝 |
Though, I'm wondering if that will result in a component that renders with close buttons that aren't usable (or, rather, that when clicked don't do anything). Is that an a11y issue? |
It would, and it is. Nothing really guarantees that a developer implementing Reflecting on this, I think it would be perfectly reasonable to mark this prop as "Required". Normally I would be concerned about the backwards-compatibility impact of removing this default prop, but since it is explicitly documented as required, I think it would be fair to let this go through as proposed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per comments, I think this is an appropriate improvement. I'd wait to merge pending @gziolo's thoughts.
Agreed, it will at least give some feedback when this prop isn't provided but the end effect will be the same. I didn't consider it. |
closes #16062
Let me know if you have any further questions/comments.