-
Notifications
You must be signed in to change notification settings - Fork 54
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
Navbar scroll: unintended scroll while tabing inside navbar #1274
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
5b22664
to
2a217ce
Compare
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.
LGTM
2a217ce
to
ffe0d88
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This comment was marked as outdated.
This comment was marked as outdated.
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.
LGTM
This comment was marked as resolved.
This comment was marked as resolved.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Everything seems OK on Chrome + Mac OS (just see my comment on the accessibility.md file). Checking on Firefox + Mac OS, there is a problem with the focus. Only the "Toggle sticky/fixed header" button gets it and none of the footer interactive elements. Solution found to restore the focus on MacOS + Firefox and on MacOS + Safari : go to the Mac Apple menu > System settings > Keyboard and to tune on the "keyboard navigation" toggle button. |
Not quite sure to understand, Do you mean in navbar sticky example ? There isn't any footer right ? |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
A keyboard parameter in Mac OS is turned off by default and prevents the focus to be active and visible on links in Safari and Firefox browsers. When turned on, the focus works properly. |
Last commit 1d48c99 updates the values and variables used. It also embeds the migration guide. |
scss/_variables.scss
Outdated
@@ -438,7 +438,7 @@ $enable-validation-icons: true !default; | |||
$enable-negative-margins: false !default; | |||
$enable-deprecation-messages: false !default; | |||
$enable-important-utilities: true !default; | |||
$enable-fixed-header: true !default; // Boosted mod: used to apply scroll-padding-top | |||
$enable-fixed-header: false !default; // Boosted mod: used to apply scroll-padding-top |
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.
Are you sure that the default value of $enable-fixed-header
should be false
?
Taking https://deploy-preview-1274--boosted.netlify.app/docs/5.3/examples/navbar-sticky/ as an example which doesn't work as expected. Maybe https://github.com/Orange-OpenSource/Orange-Boosted-Bootstrap/blob/main/site/content/docs/5.3/examples/navbar-sticky/navbar-sticky.css needs to be modified in this PR as well.
@@ -83,6 +83,10 @@ When using a fixed (or sticky) header, tabbing backward often hides focused elem | |||
1. `$scroll-offset-top` variable defines the offset, | |||
2. and [`$enable-fixed-header` allows to drop this rule]({{< docsref "/customize/options" >}}) if you don't use a fixed header. | |||
|
|||
{{< callout warning >}} | |||
Know that using this feature might lead to unexpected scroll while tabbing inside the header. Prefer `scroll-margin-*` on focusable elements. |
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.
Can't we use scroll-margin-*
all the time then?
As you said, tried something without |
Wait for twbs/bootstrap#38220 before merging!Fixes #1240.
Might be breaking but here is the solution taking into account the different navbar heights.
Check that there isn't any regression in the doc + here