-
Notifications
You must be signed in to change notification settings - Fork 4k
🚮 Merges amp-sidebar 0.2 into 0.1 and deprecates 0.2
#26177
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
Conversation
|
|
||
| if (!element.hasAttribute('role')) { | ||
| element.setAttribute('role', 'menu'); | ||
| element.setAttribute('role', 'dialog'); |
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.
This is potentially a breaking change if people use role=menu to set styling. Do we know why we're making this change? Or, is this a change that ideally only applies when amp-nested-menu is a child component of amp-sidebar?
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.
This is coming from #24382 and that's true, it's a breaking change. Not sure how to proceed here.
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.
To be on the safe side, I would recommend gating this change under an aggressively rolled out experiment (you can directly set it to 100% and monitor for two release cycles), or putting it in an isolated PR that allows a clean rollback, but feel free to use your discretion.
| element.setAttribute('role', 'menu'); | ||
| element.setAttribute('role', 'dialog'); | ||
| } | ||
| element.setAttribute('aria-modal', 'false'); |
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.
The aria change is solid.
|
Is this PR still active? |
|
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
Closes #26176, Closes #25049, Closes #25022
Changes
amp-sidebar0.2 to version 0.1amp-sidebar0.2