Simplify code related to the admin menu #4005
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 8, 2020
javierm
force-pushed
the
off_canvas
branch
2 times, most recently
from
09bbdc1
to
42d9be7
Compare
javierm
force-pushed
the
off_canvas
branch
7 times, most recently
from
4cf297e
to
5985441
Compare
javierm
force-pushed
the
off_canvas
branch
2 times, most recently
from
b7d3c3d
to
527b9d9
Compare
javierm
force-pushed
the
off_canvas
branch
3 times, most recently
from
d497032
to
1739757
Compare
It isn't used since commit e5f9cf6.
javierm
force-pushed
the
off_canvas
branch
7 times, most recently
from
a99d14b
to
b3fd14b
Compare
Senen
requested changes
Sep 21, 2020
While Foundations's off-canvas menu allows us to forget about writing CSS, it also leads to complicated HTML. Ideally Foundation would provide an easy way to simplify what we're doing, but I haven't found anything in the documentation. We could simplify the HTML a bit more if we used a CSS grid layout instead of a flex one, but old browsers have better support for the latter. Note we're using `breakpoint(medium)` so we can group the CSS for small screens and follow SCSS-Lint rules at the same time. Also note behavior of the main area when the menu appears on small screens is slightly different: it doesn't move the main content to the right. I've done it this way so we don't have any overflow issues, unlike the previous version. There's a small issue using a label and a checkbox to enable/disable the menu: sighted keyboard users with a small screen might not be able to enable the menu. So we're adding the `:focus-within` pseudoclass so the menu can be normally navigated using the keyboard. Even if old browsers don't support this pseudoclass, we believe the probability of a sighted user using a small screen, navigating with the keyboard and using an old browser is really low, particularly in the admin area. We're also adding the `aria-hidden` attribute on the label, since the menu is never hidden for screen readers and so having a control to show it could be confusing. Since the label is not focusable, we're complying with the fourth ARIA rule: > Do not use role="presentation" or aria-hidden="true" on a focusable > element . > > Using either of these on a focusable element will result in some users > focusing on 'nothing'.
The <main> tag was including the navigation, and now we use the same flex layout, making it more accessible for mobile phone users. I'm not sure the <main> tag should actually include the account info and the flash message. I'm keeping it like this in order to keep the layout the way it was.
The `side_menu` helper method isn't necessary since commit 13b3d9c.
Senen
approved these changes
Sep 21, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives