Fix menu on "wide" small screens #5072
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
javierm
force-pushed
the
top_bar_adjustments
branch
8 times, most recently
from
9b5ea84 to
d072e69
Compare
javierm
force-pushed
the
top_bar_adjustments
branch
from
d072e69 to
1240975
Compare
taitus
approved these changes
Feb 16, 2023
javierm
force-pushed
the
top_bar_adjustments
branch
from
c70af65 to
89f4bf6
Compare
This way we follow the convention of one stylesheet per component. Note that we depend on Foundation's `menu` class for some of the styles and JavaScript behavior, so we're keeping both the `menu` and `account-menu` classes.
This class was only there in order to use Foundation's styles, but the amount of styles we were using was equal to the amount of styles we were overwriting.
This is one of Foundation's classes that only applies when its parent element is a flex container, which isn't the case here since commit dcec003.
There isn't a `ul` element here, so this rule doesn't apply.
We don't need it since it's only used for flexbox styles, and we already have a `flex-grow` rule for the `h1` element which does the same thing.
javierm
force-pushed
the
top_bar_adjustments
branch
2 times, most recently
from
8ab809a to
81438b7
Compare
We're going to change these styles in order to fix a bug, and the Foundation styles were getting in the way. Besides, we were overwriting some rules and so now we're removing 6 properties while we're also adding 6, so it isn't like the Foundation styles were helping us.
The menu didn't look properly on these screens since commit dcec003. On small screens with enough horizontal space to show the menu button, the logo, and the contents of the menu, all three elements were shown on the same row, which looked broken. Now the contents of the menu are shown below the menu button. Note that, to force this, we're making the contents of the menu 100% wide. That means links would take the 100% of the space, which would make it easy to click on a link while trying to scroll when using touchscreens. So we're making the links as wide as their text, which also has a disadvantage: it's harder to click on narrow links like "SDG".
We're changing the order of the elements in the HTML so the menu button appears next to the menu it opens, with no logo between them, which IMHO makes sense and makes it easier to understand the layout for people using screen readers. A small advantage of this approach is that on very narrow screens or Consul applications having a very long word for "Menu", the menu button appeared on top, the logo appeared below it, and the contents of the menu appeared below the logo. Now the logo appears on top, the menu button appears below it, and the contents of the menu appear below the menu button.
So now it uses the same interface and styles as the main layout. On small screens, it's easier to play with the menu when the button is on the left because the menu it opens is aligned to the left. Note that now we can get rid of the title-bar class; we didn't use the styles in the public area since commit dcec003, and we were overriding all the Foundation styles in the admin area with the exception of the padding, which we no longer need.
We were using the same code for the button in both the public and admin headers, so we're removing the duplication. Since the menu and the button must go together, and the contents of the menu are different for different layouts, we're passing these contents using a block. Note the ID of the menu was `responsive-menu` in the public section but `responsive_menu` in the admin section. Since we usually use underscores for IDs and dashes for classes, we're keeping the one with the underscore.
We weren't using the `admin-top-bar` class since commit e6c1cf7, and we can always use the `.admin .top-bar` selector if we need to. And the `row expanded` classes basically give an element a width of 100%, which is already the default width for block elements.
We were using partials to render components in order to ease the transition of custom code from earlier versions of Consul. However, that was back in Consul 1.4, and now these views looked a bit messy since they sometimes rendered components and sometimes they rendered partials.
So we're consistent with the rest of the code in the header, which renders components and not partials.
That way it's possible to override the style without changing the HTML, which is the hardest code to customize and maintain.
The `overflow: hidden` applied to the `.callout` selector made the full width background invisible, meaning this section hasn't looked properly on very large screens since commit 701378d.
Instead of adding all the styles of a callout and then overwriting half of them, we can simply add the half we need.
We can use the `link` mixin. Note this mixin uses anchor-color instead of brand-color; by default, they're both the same, so we probably meant anchor-color here.
Not sure why these properties were added, but they no longer seem to be necessary.
javierm
force-pushed
the
top_bar_adjustments
branch
from
81438b7 to
1be2ab5
Compare
The top bar padding was different on small screens when we were in the admin section, so we're now applying the same padding everywhere. Note we're still applying extra padding on medium/large screens because in the public section we display our logo image, which has some blank space. In the admin section we're emulating this blank space with padding; we might change it in the future if we change our logo. Also note we're using `0.8rem` instead of `$line-height / 2`. The reason is tricky: there's a spec testing the reorder feature with drag and drop in the poll questions answers administration, and that test fails when the drop space is right at the bottom of the screen, which is what happens when we use the `$line-height / 2` padding. A proper solution would be to remove the inaccessible drag and drop feature and use a different method to reorder the answers.
javierm
force-pushed
the
top_bar_adjustments
branch
from
1be2ab5 to
f8df900
Compare
taitus
approved these changes
Feb 16, 2023
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
Visual changes
Before these changes
After these changes