Make navigation appear next to menu rather than scroll #3270
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
Codecov Report
@@ Coverage Diff @@
## master #3270 +/- ##
==========================================
+ Coverage 82.10% 82.27% +0.17%
==========================================
Files 320 320
Lines 21131 21181 +50
Branches 3196 3196
==========================================
+ Hits 17349 17427 +78
+ Misses 2739 2712 -27
+ Partials 1043 1042 -1
Continue to review full report at Codecov.
|
dracos
force-pushed
the
mobile-internal-nav
branch
6 times, most recently
from
e24aba6
to
682486e
Compare
dracos
force-pushed
the
mobile-internal-nav
branch
from
682486e
to
07dc51a
Compare
zarino
requested changes
Jan 8, 2021
web/cobrands/sass/_base.scss
Outdated
| right: 0; | ||
| background-color: rgba(0, 0, 0, 0.4); | ||
| } | ||
|
|
||
| // The nav list has this sort of "full width" look, |
There was a problem hiding this comment.
I think if we change the left: 1em; right: 1em; below to left: 0; right: 0; then we can remove the margin: 0 -1em; and this comment too.
There was a problem hiding this comment.
Would mean we could also drop a margin: 0 from the #main-nav ruleset in _layout.scss too.
dracos
force-pushed
the
mobile-internal-nav
branch
from
34fa3f5
to
67510c4
Compare
Have seen a situation (more likely with upgraded jQuery?) where the test code runs before the map has initialised its layers, so make sure the map is loaded. And make sure a couple of other things load before clicks/submissions.
References to these two files were removed in 44c97f3 but the files weren’t removed at the same time.
Duplicate in front page JS as that does not load the main file.
Improve keyboard accessibility by having it next to the opener, which uses a label and hidden checkbox to work without JavaScript.
dracos
force-pushed
the
mobile-internal-nav
branch
from
67510c4
to
289f38d
Compare
dracos
force-pushed
the
mobile-internal-nav
branch
from
289f38d
to
8af2a30
Compare
|
Added skip to content link if you could check that over, then I think this might be ready to go. |
zarino
added a commit
that referenced
this pull request
Nov 29, 2021
I am totally at a loss as to when this bug was introduced, or what caused it. The stacking clearly worked fine when the new JavaScript- powered pop-over menu was added 12 months ago (in a01e927), so something must have changed, in core, since then. I think the absolute positions of everything on the map page have always been relative to the viewport (rather than there being any `position: relative` parents). If so, it can’t be that a positioned parent has been removed, causing the `z-indexes` to no longer be in the right stacking context. The `z-index: 1` for `.map-fullscreen #map_box` was set 5 years ago (https://github.com/mysociety/fixmystreet/blame/master/web/cobrands/sass/_base.scss#L2171) in a5ef113, with a comment: `// stack above positioned elements later on the page (eg: .report-list-filters)`. The `z-index: 1` for `#main-nav` was set 12 months ago (https://github.com/mysociety/fixmystreet/blame/master/web/cobrands/sass/_base.scss#L784) in 1784886 (part of the same pop-over menu pull request #3270) and it even has an inline comment saying `// So appears over map`! Anyway, unless someone else can find what changed in the meantime, I’ve done the most obvious thing necessary, which is to define higher z-indexes for the `#js-menu-open-modal` and `#main-nav`. Fixes #3617
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.
Lets you scroll it if the window is short:
Some aside commits to tidy some things up so I don't have to change as much; then one commit that implements it without moving it in the DOM, using JS to make it open/close, then a second commit that moves it in the DOM to right after the button, which I think makes it more keyboard navigable and more sense. Probably also needs a skip to content adding then as well.