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

fix: use button instead of div for navbar toggle button #64

Merged
merged 1 commit into from
Feb 14, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 14, 2021

For improve accessibility purposes.

@lex111 lex111 requested a review from yangshun February 14, 2021 16:04
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 14, 2021
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Do we need to change this on Docusaurus classic theme side?

@yangshun yangshun merged commit e9a3efb into master Feb 14, 2021
@lex111
Copy link
Contributor Author

lex111 commented Feb 14, 2021

Yes, of course I will do it after the new release of Infima.

@lex111 lex111 deleted the lex111/navbar-toggle-a11y branch February 14, 2021 16:25
@@ -83,6 +83,9 @@
cursor: pointer;
display: none;
margin-right: 0.5rem;
border: 0;
background: 0 0;
Copy link
Contributor

@Simek Simek Feb 15, 2021

Choose a reason for hiding this comment

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

If the goal here was to reset the background position the background-position property should be used.

If the goal was to remove background completely using none is recommended over 0 and the second 0 is redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants