Fix fragment scroll margin#8702
Conversation
|
Newest code from mattermost has been published to preview environment for Git SHA 26422a6 |
cpoile
added
1: Dev Review
|
While the scroll issue is indeed fixed (thanks, @cpoile!), the right-hand page-level navigation pane isn't accurate: I've historically believed the scroll and nav issue as one and the same root cause (Furo bug). Fixing the scroll issue alone makes the nav issue much more obvious, and I'd argue that it's more egregious than the scroll issue. Note: fixing via Furo rather than forking Furo is preferred due to the long-term maintenance complexity a fork would introduce. |
|
Newest code from mattermost has been published to preview environment for Git SHA 498811a |
Thanks @cwarnermm. But the problem is in our css, not the theme. The problem is that the rules around positioning of the headers and notification bars are in a few different files, and are using absolute positioning, and that means our header is not a single block that the theme understands as our header. So its calculations are off. We have a couple options:
@lieut-data I can finish 2 up, since I spent a few hours on it already, but I'm way past a "quick fix". It's not a great use of time, but we need to fix this. But ideally we would have the team do the fix, because they should own this and make it work properly. But they're overloaded. |
|
@cpoile - I really appreciate having your expertise available for this gnarly issue that's plagued us since we introduced Furo. Any additional assistance you can offer towards as close a resolution as possible would be AMAZING and so very welcome!! I've always assumed it was the theme and not something we were doing. Happy to be incorrect about that and able to control this outcome (with help!) |
|
Newest code from mattermost has been published to preview environment for Git SHA b873914 |
|
Ok, here's the changes I have so far. There's some big issues with this code -- we're using scss, but not really, because I think the original developer (Alan Lew?) was compiling it locally, but didn't include the compilation step in CI or in scripts (maybe they were using VS Code or something). Anyway, it means any changes since have been trying to keep the sass and css in sync manually. Next steps:
Hope that helps. Sorry to leave it still in a messy state, but I think its at least a bit cleaner than before. cc @lieut-data |
|
Newest code from mattermost has been published to preview environment for Git SHA 6bb3c29 |
|
@cwarnermm gentle ping on this, thank you! |
|
Adding you @asaadmahmood per @esethna suggestion. Thanks! |
|
@asaadmahmood - Would you be able to help with a review on this one? Thanks so much in advance! |
|
@esethna - This one should be good to go. |
|
Newest code from mattermost has been published to preview environment for Git SHA 697ad32 |
📝 WalkthroughWalkthroughThe changes refactor notification bar layout and scroll positioning across stylesheets and templates. Fixed-position notification bar properties are replaced with static positioning and CSS variables for dynamic height calculations. Responsive padding rules are reorganized, and the notification bar HTML is relocated within the template structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/_static/css/mattermost-global.css (1)
218-235:⚠️ Potential issue | 🟡 MinorHover state selector doesn’t match the close control class.
The close control class is
notification-bar__close(see source/_templates/custom-nav.html Line 8), but the hover rule targets.notification-bar-hide:hover, so the intended hover feedback won’t apply.Suggested fix
-.notification-bar-hide:hover { +.notification-bar__close:hover { opacity: .5; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/_static/css/mattermost-global.css` around lines 218 - 235, The hover rule targets .notification-bar-hide:hover but the close control uses the class notification-bar__close, so the hover feedback never applies; update the CSS selector to target .notification-bar__close:hover (or include both .notification-bar__close:hover, .notification-bar-hide:hover if both classes are used) and keep the same opacity rule to ensure the close button shows the intended hover state; verify the class used in source/_templates/custom-nav.html (notification-bar__close) remains consistent with the CSS.
🧹 Nitpick comments (2)
source/_static/scss/homepage-v1.scss (1)
26-29: Remove redundant mobile override for.sidebar-sticky.The mobile rule now repeats the same
padding-top: 120pxalready applied in the parent block, so it can be dropped for clarity.Suggested cleanup
.sidebar-sticky { - `@media` (max-width: 770px) { - padding-top: 120px; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/_static/scss/homepage-v1.scss` around lines 26 - 29, Remove the redundant mobile override by deleting the nested `@media` (max-width: 770px) block that sets padding-top: 120px inside the .sidebar-sticky rule; keep the parent .sidebar-sticky declaration (which already applies padding-top: 120px) and remove the duplicate media query to avoid repetition and simplify the stylesheet.source/_static/css/homepage-v1.css (1)
413-428: Replace hardcoded120pxwith shared header variables and drop duplicate mobile rule.These offsets should track
--notification-bar-heightand--nav-bar-heightfromsource/_static/css/mattermost-global.cssto avoid drift as header values evolve.Suggested refactor
body.with-notification .toc-sticky, body.with-notification .main > .content, body.with-notification .sidebar-sticky { - padding-top: 120px; + padding-top: calc(var(--notification-bar-height) + var(--nav-bar-height) + 2px); } @@ -@media (max-width: 770px) { - body.with-notification .sidebar-sticky { - padding-top: 120px; - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/_static/css/homepage-v1.css` around lines 413 - 428, Replace the hardcoded 120px offsets in the selectors body.with-notification .toc-sticky, body.with-notification .main > .content and body.with-notification .sidebar-sticky with a computed value using the shared CSS variables (e.g. calc(var(--notification-bar-height) + var(--nav-bar-height))) so the offsets track values in mattermost-global.css, update body.with-notification .mobile-header to use the appropriate variable-based offset instead of the fixed -15px if needed, and remove the duplicate `@media` (max-width: 770px) block that repeats .sidebar-sticky padding-top.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/_static/css/mattermost-global.css`:
- Around line 170-176: The font-family declaration in .notification-bar__content
violates the font-family-name-quotes rule—only family names containing spaces
should be quoted and generic-family names must be unquoted; update the
font-family value in the font-family property of .notification-bar__content so
that multi-word names (e.g., Helvetica Neue) remain quoted, single-word families
(e.g., Lato, Arial, trade-gothic-next if it has no spaces or is registered as a
single token) are unquoted, and the generic family (sans-serif) is unquoted to
satisfy stylelint.
In `@source/_templates/custom-nav.html`:
- Line 12: The external anchor in custom-nav.html that uses target="_blank" (the
<a> element linking to the Mattermost v11 blog) is missing rel="noopener
noreferrer"; update that anchor to include rel="noopener noreferrer" to prevent
tabnabbing and ensure safer external link behavior while keeping
target="_blank".
- Around line 8-10: Replace the non-semantic anchor used for closing the
notification with a proper keyboard-accessible button: change the element with
class notification-bar__close to a <button type="button">, retain the
notification-bar__close class and any existing data attributes (e.g.,
data-ol-has-click-handler) so the existing click handler in
source/_static/js/myscript-v1.js continues to match, and add an accessible name
such as aria-label="Close notification" (keeping the inner <span
aria-hidden="true">×</span> for visual presentation).
---
Outside diff comments:
In `@source/_static/css/mattermost-global.css`:
- Around line 218-235: The hover rule targets .notification-bar-hide:hover but
the close control uses the class notification-bar__close, so the hover feedback
never applies; update the CSS selector to target .notification-bar__close:hover
(or include both .notification-bar__close:hover, .notification-bar-hide:hover if
both classes are used) and keep the same opacity rule to ensure the close button
shows the intended hover state; verify the class used in
source/_templates/custom-nav.html (notification-bar__close) remains consistent
with the CSS.
---
Nitpick comments:
In `@source/_static/css/homepage-v1.css`:
- Around line 413-428: Replace the hardcoded 120px offsets in the selectors
body.with-notification .toc-sticky, body.with-notification .main > .content and
body.with-notification .sidebar-sticky with a computed value using the shared
CSS variables (e.g. calc(var(--notification-bar-height) +
var(--nav-bar-height))) so the offsets track values in mattermost-global.css,
update body.with-notification .mobile-header to use the appropriate
variable-based offset instead of the fixed -15px if needed, and remove the
duplicate `@media` (max-width: 770px) block that repeats .sidebar-sticky
padding-top.
In `@source/_static/scss/homepage-v1.scss`:
- Around line 26-29: Remove the redundant mobile override by deleting the nested
`@media` (max-width: 770px) block that sets padding-top: 120px inside the
.sidebar-sticky rule; keep the parent .sidebar-sticky declaration (which already
applies padding-top: 120px) and remove the duplicate media query to avoid
repetition and simplify the stylesheet.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b38cc10-afa1-495a-8b04-5d1aab5d6e4b
📒 Files selected for processing (5)
source/_static/css/homepage-v1.csssource/_static/css/mattermost-global.csssource/_static/scss/homepage-v1.scsssource/_static/scss/partials/_header.scsssource/_templates/custom-nav.html
💤 Files with no reviewable changes (1)
- source/_static/scss/partials/_header.scss
| .notification-bar__content { | ||
| padding: 0.5rem 3rem 0.5rem 1rem; | ||
| display: flex; | ||
| align-items: center; | ||
| padding: 0 1rem; | ||
| font-family: "trade-gothic-next", "Lato", "Helvetica Neue", "Arial", sans-serif; | ||
| font-weight: 700; | ||
| font-size: 15px; |
There was a problem hiding this comment.
Fix Stylelint font-family-name-quotes at Line 174.
This declaration currently violates the configured lint rule and should be normalized to pass CI.
Suggested fix
- font-family: "trade-gothic-next", "Lato", "Helvetica Neue", "Arial", sans-serif;
+ font-family: trade-gothic-next, Lato, "Helvetica Neue", Arial, sans-serif;🧰 Tools
🪛 Stylelint (17.9.0)
[error] 174-174: Expected no quotes around "trade-gothic-next" (font-family-name-quotes)
(font-family-name-quotes)
[error] 174-174: Expected no quotes around "Lato" (font-family-name-quotes)
(font-family-name-quotes)
[error] 174-174: Expected no quotes around "Arial" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/_static/css/mattermost-global.css` around lines 170 - 176, The
font-family declaration in .notification-bar__content violates the
font-family-name-quotes rule—only family names containing spaces should be
quoted and generic-family names must be unquoted; update the font-family value
in the font-family property of .notification-bar__content so that multi-word
names (e.g., Helvetica Neue) remain quoted, single-word families (e.g., Lato,
Arial, trade-gothic-next if it has no spaces or is registered as a single token)
are unquoted, and the generic family (sans-serif) is unquoted to satisfy
stylelint.
| <a class="notification-bar__close" data-ol-has-click-handler=""> | ||
| <span aria-hidden="true">×</span> | ||
| </a> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'notification-bar__close|notification-bar-hide|querySelector\\(|addEventListener\\('Repository: mattermost/docs
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for notification-bar related JS handlers with corrected regex
rg -n 'notification-bar__close|notification-bar-hide' -A 3 -B 3Repository: mattermost/docs
Length of output: 1899
🏁 Script executed:
#!/bin/bash
# Check the full context of the custom-nav.html file around lines 6-15
fd 'custom-nav.html' --exec cat -n {} \;Repository: mattermost/docs
Length of output: 36298
🏁 Script executed:
#!/bin/bash
# Search for event listeners and querySelector patterns that might target notification-bar
rg -n 'querySelector|addEventListener|data-ol-has-click-handler' --type htmlRepository: mattermost/docs
Length of output: 1704
Use a semantic close button with a keyboard-accessible control and announced purpose.
Line 8 currently uses an <a> element without an href attribute, which is not keyboard-focusable by default and lacks an accessible name for assistive technologies. Use a <button type="button"> with aria-label to ensure the close control is properly focused and announced.
Suggested fix
- <a class="notification-bar__close" data-ol-has-click-handler="">
+ <button type="button" class="notification-bar__close" aria-label="Close notification banner" data-ol-has-click-handler="">
<span aria-hidden="true">×</span>
- </a>
+ </button>The click handler in source/_static/js/myscript-v1.js uses a class-based selector ($('body').on('click', '.notification-bar__close', ...)), so this change is compatible without requiring JavaScript modifications.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a class="notification-bar__close" data-ol-has-click-handler=""> | |
| <span aria-hidden="true">×</span> | |
| </a> | |
| <button type="button" class="notification-bar__close" aria-label="Close notification banner" data-ol-has-click-handler=""> | |
| <span aria-hidden="true">×</span> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/_templates/custom-nav.html` around lines 8 - 10, Replace the
non-semantic anchor used for closing the notification with a proper
keyboard-accessible button: change the element with class
notification-bar__close to a <button type="button">, retain the
notification-bar__close class and any existing data attributes (e.g.,
data-ol-has-click-handler) so the existing click handler in
source/_static/js/myscript-v1.js continues to match, and add an accessible name
such as aria-label="Close notification" (keeping the inner <span
aria-hidden="true">×</span> for visual presentation).
| <span aria-hidden="true">×</span> | ||
| </a> | ||
| <div class="notification-bar__wrapper"> | ||
| <a href="https://mattermost.com/blog/mattermost-v11-powering-more-mission-critical-collaboration/" target="_blank" class="notification-bar__link"><img src="{{ pathto('_static/images/notification-bar/icon-megaphone.svg', 1) }}" alt="">Mattermost v11.0 is now available! Learn what's new »</a> |
There was a problem hiding this comment.
Add rel for external target="_blank" links.
Line 12 opens a new tab without rel="noopener noreferrer", which weakens tabnabbing protections.
Suggested fix
- <a href="https://mattermost.com/blog/mattermost-v11-powering-more-mission-critical-collaboration/" target="_blank" class="notification-bar__link"><img src="{{ pathto('_static/images/notification-bar/icon-megaphone.svg', 1) }}" alt="">Mattermost v11.0 is now available! Learn what's new »</a>
+ <a href="https://mattermost.com/blog/mattermost-v11-powering-more-mission-critical-collaboration/" target="_blank" rel="noopener noreferrer" class="notification-bar__link"><img src="{{ pathto('_static/images/notification-bar/icon-megaphone.svg', 1) }}" alt="">Mattermost v11.0 is now available! Learn what's new »</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="https://mattermost.com/blog/mattermost-v11-powering-more-mission-critical-collaboration/" target="_blank" class="notification-bar__link"><img src="{{ pathto('_static/images/notification-bar/icon-megaphone.svg', 1) }}" alt="">Mattermost v11.0 is now available! Learn what's new »</a> | |
| <a href="https://mattermost.com/blog/mattermost-v11-powering-more-mission-critical-collaboration/" target="_blank" rel="noopener noreferrer" class="notification-bar__link"><img src="{{ pathto('_static/images/notification-bar/icon-megaphone.svg', 1) }}" alt="">Mattermost v11.0 is now available! Learn what's new »</a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/_templates/custom-nav.html` at line 12, The external anchor in
custom-nav.html that uses target="_blank" (the <a> element linking to the
Mattermost v11 blog) is missing rel="noopener noreferrer"; update that anchor to
include rel="noopener noreferrer" to prevent tabnabbing and ensure safer
external link behavior while keeping target="_blank".
|
@cpoile just circling back on this. It has been a while since this was opened, can you help A) summarize the issues this is solving, and B) help us understand what state this PR is in (based on the last comment it seems there was still more work to do), and C) if it's ready to merge, can you help identify what areas of the docs we should pay spacial attention to when testing these changes to ensure there are no regressions? Thanks! cc// @Combs7th |
|
Newest code from mattermost has been published to preview environment for Git SHA 85b4de6 |
|
@esethna The current state is:
But it's been that way for awhile, so pushing this won't make things worse (it improves things a bit). Last time I checked it worked on the few pages I tried, but I would feel more confident if someone who know what it should look like went through and checked (with the banners we use on and off, and across desktop sizes and mobile). The changes affect 1) the notification and headers layout, 2) the location we scroll to when we click an anchor (hash) link from outside and land on the page (it should scroll right to the section, but not past it, which is what we fixed here, 3) the sidebar table of contents should correctly highlight the section you're on (that was a bit broken so that it only showed your section /after/ the section header had gone above your viewport off screen). Hope that helps! |
|
Thanks for the info! @Combs7th can you help do some testing in the preview environment #8702 (comment)) around these areas identified above to make sure things are functioning as expected and there's nothing clearly broken? |
esethna
dismissed
Combs7th’s stale review
Dismissing so we can do testing in the areas identified above
6 participants
Summary
Ticket Link