Finishing touches with regard to header improvements#15683
Finishing touches with regard to header improvements#15683andreslucena merged 21 commits intodevelopfrom
Conversation
|
@alecslupu as this PR will have 18 fixes (!!!), I kindly ask you to be really clear in the commit messages (basically copy-pasting or rephrasing the fix from the issue) or just create a new PR for each fix. If not I'm affraid that this will be a nightmare to actual review, and you'll need to do the work two times (extracting the fixes in multiple PRs for instance once we realize that the changes are not easy to review and take too much time). |
|
Feel free to rewrite the current git commit history until it is ready for a review, as I didn't review it yet. |
I will do it. Allow me to get back to this (waiting for Frontend). |
That'd be great! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@decidim-core/app/packs/src/decidim/controllers/main_menu/controller.js`:
- Around line 92-97: closeMenu currently restores document.body.style.overflow
to this.previousBodyOverflow and then immediately overwrites it with "scroll";
change the overflow assignment so it uses the saved value with a fallback
instead of always forcing "scroll" — i.e. in closeMenu update the single
overflow assignment to set document.body.style.overflow =
this.previousBodyOverflow ?? "scroll" (remove the separate assignment), keeping
the existing aria attribute updates on this.element and this.menuContainer
intact.
- Around line 83-86: The lint failure is due to comparing
this.previousBodyOverflow directly to undefined; update the check in the
openMenu method so it uses a typeof test instead (e.g., check if typeof
this.previousBodyOverflow === "undefined") to satisfy the linter and avoid
direct undefined comparison while preserving the existing logic that assigns
document.body.style.overflow to this.previousBodyOverflow only when it hasn't
been set.
🧹 Nitpick comments (1)
decidim-core/app/views/layouts/decidim/header/_main_links_mobile_account.html.erb (1)
41-53: Consider modernizing deprecatedkeyCodeusage.The
keyCodeproperty is deprecated. While still functional, usingevent.keyis the modern approach and more readable.♻️ Suggested modernization
closeDiv.addEventListener('keydown', function(e){ - if (e.keyCode === 13 || e.keyCode === 32) { + if (e.key === 'Enter' || e.key === ' ') { menuDropdown.removeAttribute('aria-modal'); menuDropdown.setAttribute('aria-hidden', "true"); dropdownTrigger.focus(); } })Additionally, this inline script could be migrated to a Stimulus controller to align with the broader refactoring effort in this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@decidim-core/app/packs/src/decidim/controllers/assign_role/assign_role.test.js`:
- Around line 48-61: The test sets a spy on global.setTimeout before switching
to fake timers, so the spy wraps the real function and is bypassed when
jest.useFakeTimers() replaces setTimeout; to fix, call jest.useFakeTimers()
before creating the spy (i.e., move jest.spyOn(global, "setTimeout") to after
jest.useFakeTimers()), keep the rest of the test logic around
controller.disconnect() / controller.connect(), and ensure timers are advanced
and assertions run as before (optionally restore timers/spies after the test).
decidim-core/app/packs/src/decidim/controllers/assign_role/assign_role.test.js
Show resolved
Hide resolved
|
@andreslucena , let me know if the |
|
There's still a scroll weird behavior in this other menu: 
Can you fix it too please? Thanks! |
|
When the menu is opened in desktop, clicking outside of it doesn't get it closed: 
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@decidim-core/app/packs/stylesheets/decidim/_header.scss`:
- Around line 130-132: The dropdown backdrop (&__dropdown) uses "absolute top-0
right-0" so it only sizes to content; change it to cover the full viewport by
using a fixed, full-bleed positioning (e.g. replace "absolute top-0 right-0"
with "fixed inset-0") or alternatively ensure "left-0 w-full" plus the existing
"h-screen" and a high z-index so the overlay covers the whole viewport and
prevents interaction outside the menu; update the `@apply` on &__dropdown
accordingly.
🧹 Nitpick comments (1)
decidim-core/app/packs/src/decidim/controllers/main_menu/controller.js (1)
99-107: Avoid restoring a stale overflow value between openings.
previousBodyOverflowis captured only once, so if another overlay updatesdocument.body.style.overflowbetween menu openings, closing the menu could restore an outdated value and re-enable scroll unexpectedly. Capturing on each open avoids that drift.♻️ Suggested change
openMenu() { - if (typeof this.previousBodyOverflow === "undefined") { - this.previousBodyOverflow = document.body.style.overflow; - } + this.previousBodyOverflow = document.body.style.overflow document.body.style.overflow = "hidden" this.element.setAttribute("aria-expanded", "true") this.menuContainer.setAttribute("aria-hidden", "false") this.menuContainer.setAttribute("aria-modal", "true") }
|
@andreslucena this should be ready for another round ... |
|
Merging with the failure of codecov |
🎩 What? Why?
In this PR, i am handling issues reported by #15299.
arrow_linkhelper should be renamed to better reflect its new behaviour📌 Related Issues
Link your PR to an issue
Testing
Describe the best way to test or validate your PR.
📷 Screenshots
Please add screenshots of the changes you are proposing
Summary by CodeRabbit
New Features
Bug Fixes
Tests