Skip to content

Content - Replace twirldown expand/collapse icon with chevron - #4320

Open
geodem127 wants to merge 2 commits into
devfrom
feat/4232-twirldown-icon-visibility
Open

Content - Replace twirldown expand/collapse icon with chevron#4320
geodem127 wants to merge 2 commits into
devfrom
feat/4232-twirldown-icon-visibility

Conversation

@geodem127

Copy link
Copy Markdown
Contributor

Summary

  • Replaces the Content nav tree's expand/collapse icon (ArrowDropDownRounded/ArrowRightRounded) with a chevron pair (KeyboardArrowDownRounded/KeyboardArrowRightRounded), per the direction agreed in Manger UI - Increase Visibility of Twirldown Icon #4232 after enlarging the existing icon was tried and rejected.
  • Adds an explicit base-state icon color (action.active) alongside the existing selected-state override, matching the precedent already shipped in Studio's layers panel (StudioLayersTreeItem.tsx).
  • Change lives entirely in the shared NavTree component, so it applies to all five consumers (Content, Schema, Media, Code, Settings) — no per-app scoping, since nothing in the issue thread restricted it to Content only.

Fixes #4232


Screen Recording

Screen.Recording.2026-09-03.at.10.22.36.AM.mov

Requirement → Verification traceability

Requirement (from issue #4232) Implementation Verified by
Replace rounded arrow icons with a chevron pair src/shell/components/NavTree/index.tsxslots.collapseIcon/expandIcon swapped to KeyboardArrowDownRoundedIcon/KeyboardArrowRightRoundedIcon Live DOM check: data-testid=KeyboardArrowDownRoundedIcon; screenshot of rendered chevron
Do not enlarge — keep current footprint (explicitly rejected by maintainer) No fontSize override added; icon left at MUI default Row height measured at 28px (normal single-line), no layout push in screenshots
Selected-row icon color still works RichTreeItem.tsx — added base action.active rule alongside, not replacing, the existing .Mui-selectedprimary.main override; CSS-specificity checked (.Mui-selected combinator wins) Measured computed color rgb(255, 93, 10) on a selected row
expansionTrigger="iconContainer" still holds — icon expands, label navigates No prop touched aria-expanded toggles on icon click; unchanged on label click
No regression in shared consumers Change is global by design (single config point) Cypress content/navigation.spec.js (8 passing/1 pending) and code/sidebar.spec.js (1 passing) re-run post-change, identical to pre-change baseline
TypeScript correctness npx tsc --noEmit clean, 0 errors
Verified visually against a page with child items Playwright screenshot + video against seeded item /content/6-bac9f3bcbe-b7s4bv/7-f88ad8bcc3-1z83ht

Diff

  • src/shell/components/NavTree/index.tsx — icon imports + slots config (4 lines changed)
  • src/shell/components/NavTree/components/RichTreeItem.tsx — base-state icon color rule (+4 lines)

No logic, layout, or API changes.

Open item for reviewer

The issue never explicitly confirmed whether this should apply to Content only or globally to all NavTree consumers (Schema/Media/Code/Settings). This PR defaults to global, since the icon slot is configured in one shared place and nothing in the thread pushed back on a broader change. Flag if Content-only was actually intended — reverting to scoped would require forking the icon config per consumer.

Test plan

  • cypress/e2e/content/navigation.spec.js — passing
  • cypress/e2e/code/sidebar.spec.js — passing
  • npx tsc --noEmit — clean
  • Manual/automated visual verification (Playwright) against Content tab, expand/collapse/select interactions
  • Reviewer: spot-check Schema, Media, Settings nav trees for visual acceptability (not covered by existing Cypress specs)

@geodem127
geodem127 requested a review from agalin920 September 3, 2026 02:26
@geodem127 geodem127 self-assigned this Sep 3, 2026
@geodem127 geodem127 added the enhancement Improvement to an existing feature label Sep 3, 2026
@geodem127
geodem127 requested a review from finnar-bin September 3, 2026 02:27
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

QA Review — ⚠️ INCONCLUSIVE

Validates #4232: Manger UI - Increase Visibility of Twirldown Icon

  1. ✅ Twirldown icon swapped to a chevron pair, matching the direction the maintainer agreed on in the issue thread after rejecting the "just enlarge" approach
  2. ✅ Both expand (KeyboardArrowRightRounded) and collapse (KeyboardArrowDownRounded) icons updated consistently in the same slots config
  3. ✅ Selected-row icon color still works — the existing .Mui-selected override in RichTreeItem.tsx is left untouched and retains higher CSS specificity than the new base-state color rule
  4. ⚠️ Improved prominence / no layout regression — no size or spacing overrides were added alongside the icon swap, but whether the new chevron reads as sufficiently more prominent, and whether row alignment holds, can't be judged from static code
  5. ⚠️ Scope decision (Content-only vs. all NavTree consumers) — the issue thread never explicitly settled this; the PR applies the change globally via the shared component, which the author flags as an open item rather than a recorded decision
Suggested Cypress coverage

cypress/e2e/content/navigation.spec.js — add a case that asserts the expand/collapse icon on a nav-tree row with children renders the chevron variant (e.g. via data-cy/data-testid on the slot icon) rather than the old arrow icon, covering both the expanded and collapsed states. Add a case confirming clicking the icon toggles expansion while clicking the row label still navigates (guarding the expansionTrigger="iconContainer" behavior). Add a case selecting a row with children and asserting the icon color reflects the selected state. Since the icon slot change is shared, consider a lightweight equivalent assertion in cypress/e2e/code/sidebar.spec.js (or the Schema/Media/Settings sidebar specs) to catch a regression in a non-Content tree.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Negative QA

No reproducible edge-case failures found on the surfaces this PR changes.

Also checked and working correctly
  • Scope: this PR only touches src/shell/components/NavTree/index.tsx (swaps ArrowDropDownRounded/ArrowRightRounded for KeyboardArrowDownRounded/KeyboardArrowRightRounded as the tree's expand/collapse icons) and RichTreeItem.tsx (adds an explicit color: action.active rule for .MuiTreeItem-iconContainer svg, overridden to primary.main when selected). NavTree is shared across Content, Schema, Media, Code, and Settings sidebars.
  • Content, Schema, and Settings sidebars on this fixture instance (qa_negative_* models) have no nested/directory tree items, so no expand/collapse chevron renders there at all — nothing to regress.
  • Code editor's file tree (FileNav, isDirectoryNavigation) is the one surface with real nested tree nodes. Created a new view at qa_neg_folder/nested_view to force a directory node — the new chevron renders clearly and visibly (this is the fix's actual purpose), toggles correctly between expand/collapse states, and switches color correctly when the contained file is selected.
  • Rapid double-click on the folder's expand/collapse control toggled twice with no console errors and no stuck/inconsistent expand state.
  • Directly loading a URL for a file nested inside a collapsed-by-default folder correctly auto-expands the parent folder and highlights the selected file.
  • Checked console and network logs after each interaction — only pre-existing, unrelated noise appeared (media-manager /bins 404s, a GA4 401, and an MUI SSR pseudo-class warning), nothing tied to the tree/icon change.
  • Media folder creation dialog blocks creating a root-level folder when no folders exist yet (Parent Folder has "No options" and Create stays disabled) — pre-existing behavior unrelated to this diff, not exercised further since it prevented building a nested Media folder tree to test.

@agalin920

Copy link
Copy Markdown
Contributor

@geodem127

if you look at the QA check this seems to be very important

Scope decision (Content-only vs. all NavTree usages) recorded and matched — the issue's triage flagged this as an open blocking question, but neither the PR nor the issue comments record an explicit decision; the diff changes the shared NavTree/RichTreeItem components used by Content, Schema, Media, Code, and Settings, so all five trees are affected.

@geodem127

geodem127 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@geodem127

if you look at the QA check this seems to be very important

Scope decision (Content-only vs. all NavTree usages) recorded and matched — the issue's triage flagged this as an open blocking question, but neither the PR nor the issue comments record an explicit decision; the diff changes the shared NavTree/RichTreeItem components used by Content, Schema, Media, Code, and Settings, so all five trees are affected.

@agalin920 — since the icon change is in the shared NavTree/RichTreeItem components, it's not scoped to Content only. I'd suggest we include all apps that consume this tree component (Content, Schema, Media, Code, Settings) since they're all using the same icon, and the current icon produces the same visual problems across all of them as it does in Content — that's why it's necessary to implement the fix in all apps rather than limiting it to Content.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Manger UI - Increase Visibility of Twirldown Icon

2 participants