fix(menu): hide items of a submenu whose visible is false - #1657
Open
rene-schakmann wants to merge 1 commit into
Open
rene-schakmann wants to merge 1 commit into
rene-schakmann wants to merge 1 commit into
Conversation
A child item that set `visible: true` rendered even when its parent submenu had `visible: false`, because the guard allowed any child with a defined `visible`. The group header is gated on the submenu alone, so the force-shown item was left with no group label at all. Move the child loop inside the existing submenu visibility check, so a hidden group hides its header, separators and items alike, matching the non-grouped branch. This also fixes the child separator guard, which used `||` and therefore rendered separators inside a hidden group. Fixes openng-org#1648
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #1648
Problem
A child item that set
visible: truerendered even when its parent submenu hadvisible: false:The
item.visible !== undefinedclause force-shows any child that declaresvisible. The group header (li[data-pc-section="submenulabel"]) is gated onsubmenu.visible !== falsealone, so it does not render — the force-shown item is left with no group label at all, which is the a11y gap reported in #1648.The sibling separator guard had a related bug, an
||where it needs&&:With a hidden group and a separator that leaves
visibleundefined,item.visible !== falseistrue, so separators rendered inside a hidden group.Fix
Move the child loop inside the
@if (submenu.visible !== false)wrapper added in #1632 and drop the parent checks from the inner conditions. A hidden group now hides its header, its separators and its items alike, and the grouped branch reads exactly like the non-grouped one.The item
libindings,menuitemId()ids,data-pc-sectionvalues and PT hooks are unchanged.git log -Straces the escape hatch back to the pre-fork PrimeNG rename (0c463b7410) — it is undocumented and was never a deliberate feature here.Behavior change worth a look:
item.visible: trueno longer overrides a hidden parent group. Anyone relying on the force-show loses those items.Tests
Three cases added next to the ones from #1632:
visible: truechild renders no items and no orphan (fails before the fix:expected 2 to be 1)expected 1 to be +0)visible: truechild of a visible group still renders, so the fix does not overreachMenu suite: 131 passed. Full
optimus-uisuite: 7281 passed, 90 skipped, 0 failed.Verified in the docs app
Group demo temporarily patched with the issue's repro model (
Documentsgroupvisible: false, childForced Visible Itemvisible: true), reverted afterwards.Before:
Forced Visible Itemrenders on its own, noDocumentsheader above it.After: the entire hidden group is gone, only the
Profilegroup remains.Follow-up
isItemHidden()(menu.ts:942) is dead code and carries its own inverted separator logic. Left untouched here; happy to remove it separately if you want it gone.Co-authored by Claude. (The trailer is in the description rather than the commit because commitlint's
no-ai-co-authorrule rejects it in the message.)