-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
tutorial: update hamburger component to use button element #12171
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sebastian Beltran <[email protected]>
✅ Deploy Preview for astro-docs-2 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Thanks for snagging the issue! Just noting that after this specific code is finalized, we need to:
|
I just checked and in this repository there are no other changes besides the i18n one. I’m not sure if I should update it since it will be marked as updated if I make the change. I’ll make the change in the repo you shared with me |
Just answering this here as sometimes people use past PRs for reference or guidance!
Just like our PR template says, changes should only be made in one language. If we're changing the English content, we don't want any other translations changed. (And, in the rare situations where this might be helpful, we will say so and ask for it to be added alongside the English content.) |
Thanks @bjohansebas, since we are already improving accessibility, I believe we can go all the way and get the most accessible implementation. Could you follow this article along and match the implementation? I believe all the changes needed will be:
|
Hey, I made several changes to the template (withastro/blog-tutorial-demo#44), please review them and I’ll make the content changes here once we’re in agreement. |
Heads up that we seem to be happy with the changes in the blog post code PR, so I think these docs can be updated any time! (I've added some extra context to the PR description.) Are you still up for leading this, @bjohansebas ? |
Cool, yep I’ll do it this weekend, or if I find some free time before, I’ll do it then |
Just pinging this one for freshness, and to make sure you still have availability to work on this @bjohansebas ! It's no problem if you'd like or prefer reinforcements to jump in and help here! 🙌 |
Oh, yes, I’m on it right now, it’s been a very busy few weeks. |
… files Signed-off-by: Sebastian Beltran <[email protected]>
…ponent Signed-off-by: Sebastian Beltran <[email protected]>
``` | ||
|
||
2. Add the icon to `Header.astro` so that it will be displayed on all pages. Don't forget to import the component. | ||
2. Add the icon to `Header.astro` so that it will be displayed on all pages. Wrap the two buttons (ThemeIcon and Menu) inside a `<div>` to group them together, and include some basic styles to improve the layout. Don't forget to import the component. |
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 I split this into more steps, or is it fine as it is? Also, any suggestions about the message are always welcome.
:::note | ||
When you update your site to include dark mode, some colors used may need updating. Always check your rendered site when adding new styles and colors, and make adjustments when necessary! This can mean adding more `.dark` CSS style rules to display different styles in dark mode, or you may wish to update some colors so that they work equally well in both themes. | ||
|
||
Consider using accessibility tools such as a contrast checker when creating a set of colors for your site, or running a check on your website, for example with a browser extension, to spot any potential issues. |
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.
Do we want to recommend a contrast checker? I don't know any to suggest, the one built into the browser is probably sufficient
Description (required)
The hamburger component example was updated to use a button, and some classes were added to make it like the original, since by default buttons have certain styles that depend on the browser/operating system
Related issues & labels (optional)
Closes There are accessibility problems with the sample code #12143
Suggested label: a11n
Code changes: improve menu accessibility blog-tutorial-demo#44
UPDATE
This PR now tackles more accessibility issues than just the Hamburger menu, and also focuses on the Theme Toggle component and dark mode styling in general.
When updating docs to match the corresponding code changes (all instructions, code snippets should now guide people towards building the new code instead), the key things to search for to figure out where updates are needed (in code snippets and text/explanation/instructions) are:
Hamburger.astro
has been renamed toMenu.astro
)Sections requiring changes based on the updated code
A scan through the tutorial on Talking and Doc'ing shows changes are required (sometimes extensively) in the following pages:
skillColor
variable colour updated when styling the about page (navy -> crimson)