-
Notifications
You must be signed in to change notification settings - Fork 554
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 offcanvas menu open bug #1116
Conversation
Add the click handler to the backdrop element instead of the document to close the offcanvas menu on mobile devices. Fixes php#1112
🚀 Regression report for commit 4c23310 is at https://web-php-regression-report-pr-1116.preview.thephp.foundation |
🚀 Preview for commit 4c23310 can be found at https://web-php-pr-1116.preview.thephp.foundation |
Sets the initial focus on the first navigation item instead of the close button when opening the offcanvas nav. Improves accessibility and user experience for keyboard and screen reader users. Closes php#1118
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.
LGTM. Thank you!
js/common.js
Outdated
} | ||
}; | ||
} |
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.
Should this semicolon be retained?
} | |
}; |
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.
Yes! Some other lines were also missing semicolons, so I just ran this navbar section through Prettier again.
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.
Maybe we should employ a linter?
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.
Thanks!
Linter is a great proposal, I guess we can add it in a separate PR
Add the click handler to the backdrop element instead of the document to close the offcanvas menu on mobile devices.
Fixes #1112, fixes #1118