fix(docgen-sidebar): modernized blueprint dropdown (DOCGEN-7872) #4392
Conversation
WalkthroughRemoves inline indentation from TagTree Accordion items, centralizes indentation and other presentation via updates to DocGenSidebar SCSS, and adds Storybook stories (with MSW handlers) demonstrating DocGenSidebar variants using mock data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (3)
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss (2)
31-36: Quoted CSS values here are invalid and likely ignored
width: '100%',min-width: 'fit-content',height: '100%', andoverflow: 'auto' 'auto'are not valid CSS value syntaxes and will be ignored by the browser. If this wrapper’s sizing/overflow still matters, consider normalizing to unquoted, valid values in a follow‑up:-.bcs-TagsSection-accordion-wrapper { - width: '100%'; - min-width: 'fit-content'; - height: '100%'; - overflow: 'auto' 'auto'; -} +.bcs-TagsSection-accordion-wrapper { + width: 100%; + min-width: fit-content; + height: 100%; + overflow: auto; +}
53-62: Considertext-transform: noneinstead ofunsetfor reliabilitySetting
--accordion-min-width: 0andwidth: 100%on.bcs-DocGen-collapsibleis a good way to prevent Blueprint’s layout constraints from clipping the dropdown.For the descendant rule:
.bcs-DocGen-collapsible { … * { text-transform: unset; } }
text-transformis an inherited property;unsethere effectively behaves likeinherit, which may not override an uppercase style applied higher up. To reliably keep labels in their original case, consider:- * { - text-transform: unset; - } + * { + text-transform: none; + }This ensures text is not force‑uppercase regardless of ancestor styles.
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js (1)
24-55: Factor out shareddocGenSidebarPropsto remove duplication
basicandwithModernizedBlueprintshare identicaldocGenSidebarProps. To keep the stories easier to maintain if this shape changes, consider extracting a shared constant:- export const basic = { - args: { - defaultView: 'docgen', - docGenSidebarProps: { - enabled: true, - isDocGenTemplate: true, - checkDocGenTemplate: () => {}, - getDocGenTags: () => - Promise.resolve({ - pagination: {}, - data: mockDocGenTags, - }), - }, - }, - }; +const docGenSidebarProps = { + enabled: true, + isDocGenTemplate: true, + checkDocGenTemplate: () => {}, + getDocGenTags: () => + Promise.resolve({ + pagination: {}, + data: mockDocGenTags, + }), +}; + +export const basic = { + args: { + defaultView: 'docgen', + docGenSidebarProps, + }, +}; export const withModernizedBlueprint = { args: { enableModernizedComponents: true, defaultView: 'docgen', - docGenSidebarProps: { - enabled: true, - isDocGenTemplate: true, - checkDocGenTemplate: () => {}, - getDocGenTags: () => - Promise.resolve({ - pagination: {}, - data: mockDocGenTags, - }), - }, + docGenSidebarProps, }, };Functionally identical, but reduces the chance of the two stories drifting apart unintentionally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (3)
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss(2 hunks)src/elements/content-sidebar/DocGenSidebar/TagTree.tsx(0 hunks)src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/elements/content-sidebar/DocGenSidebar/TagTree.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
Applied to files:
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss (1)
38-40: Tag path base padding looks consistent with overridesDefining a base
padding-leftfor.bcs-DocGen-tagPathand then zeroing it out within.bcs-DocGen-accordionfor the dropdown context is a reasonable pattern and keeps indentation logic in CSS instead of inline styles.src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js (2)
1-22: Imports and default story args look consistent with existing patternsUsing
ContentSidebaras the component withdefaultArgswired toglobal.FEATURES,global.FILE_ID, andglobal.TOKENmatches how other stories in this repo typically bootstrap the sidebar. No issues from a Storybook/Flow perspective as long as those globals are defined in the Storybook environment.
57-70: MSW integration and default export are wired correctlyThe default export (title, component, args) and the MSW handler that returns
mockFileRequest.responseformockFileRequest.urlform a solid baseline for exercising the DocGen sidebar in isolation. This should give you stable visual stories for both legacy and modernized Blueprint layouts.
pyaromchyk-stack
force-pushed
the
DOCGEN-7872
branch
from
31b68ea to
290f02c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss (1)
32-35: Pre-existing: Invalid quoted CSS values.Lines 32-35 contain syntax errors with quoted CSS values (
'100%','fit-content','auto' 'auto'). These should be unquoted. While not introduced by this PR, they could prevent proper styling.Apply this diff to fix the syntax errors:
.bcs-TagsSection-accordion-wrapper { - width: '100%'; - min-width: 'fit-content'; - height: '100%'; - overflow: 'auto' 'auto'; + width: 100%; + min-width: fit-content; + height: 100%; + overflow: auto; }Would you like me to open a separate issue to track fixing these pre-existing errors?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss(2 hunks)src/elements/content-sidebar/DocGenSidebar/TagTree.tsx(0 hunks)src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/elements/content-sidebar/DocGenSidebar/TagTree.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss (2)
38-40: LGTM! Padding structure supports tag hierarchy.The new global padding rule for
.bcs-DocGen-tagPathworks well with the existing override at line 48 that zeros out padding for nested tags within accordions. This creates the proper visual hierarchy.
54-61: LGTM! Changes address the overflow/alignment issues.The combination of
min-width: 0,width: 100%, and unsettingtext-transformare appropriate fixes for the Blueprint modernization issues described in the PR:
min-width: 0allows flex/grid children to shrink below their content sizewidth: 100%ensures the collapsible takes full available width- Unsetting
text-transformremoves unwanted inherited styling from Blueprintsrc/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js (2)
1-22: LGTM! Standard Storybook setup with appropriate configuration.The imports and defaultArgs are well-structured. Using global variables for features, fileId, and token is appropriate for Storybook stories.
57-70: LGTM! MSW handler properly mocks the file request.The Storybook configuration follows best practices with proper MSW setup for HTTP mocking. The handler correctly returns the mock file request response as JSON.
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx
Show resolved
Hide resolved
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.js
Outdated
Show resolved
Hide resolved
| * { | ||
| text-transform: unset; | ||
| } |
There was a problem hiding this comment.
which element(s) are we trying to target? we should avoid using a wildcard like this unless necessary
There was a problem hiding this comment.
These dropdown headers ("about") and dropdown items ("item") all should be in the original formating
There was a problem hiding this comment.
got it I see, your change makes sense then. but I also think that we'll need to fix Blueprint eventually because this seems like a weird behavior and has internalization issues
There was a problem hiding this comment.
@pyaromchyk-stack could you open an issue for blueprint and let them know there's a concern with setting text-transform: uppercase?
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx (1)
52-65: Simplify themetatyping usingsatisfies.The current intersection type approach is heavier than necessary. Since your project uses TypeScript 5.2.2, use
satisfies Meta<typeof ContentSidebar>instead, which will provide proper type inference and validation while keeping the code cleaner. This pattern is already used elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-11T14:43:02.677Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Applied to files:
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx
🧬 Code graph analysis (1)
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx (1)
src/elements/content-sidebar/stories/ContentSidebar.stories.js (1)
defaultArgs(6-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx (1)
27-35:checkDocGenTemplate: noopis a solid mock choice (handles any-args signature).
This avoids brittle “wrong arity” mocks and keeps the story minimal.
src/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsx
Outdated
Show resolved
Hide resolved
pyaromchyk-stack
force-pushed
the
DOCGEN-7872
branch
from
8c2c316 to
8a3e853
Compare
| * { | ||
| text-transform: unset; | ||
| } |
There was a problem hiding this comment.
@pyaromchyk-stack could you open an issue for blueprint and let them know there's a concern with setting text-transform: uppercase?
There was a problem hiding this comment.
I see quite of few blueprint overrides in this file. in general, we should avoid overriding blueprint because it can potentially lead to issues such as what caused this PR
There was a problem hiding this comment.
Let blueprint team know about case issue
Since I am not authorized to merge tin this project, would you mind helping me with that?
3 participants
After blueprint's component modernization update DocGen's side bar started to look misaligned to the left, the dropdown items were pushed outside the container. This PR I adjusts the look of DocGen tags dropdown and adds stories with this component.
Ticket link
Before:
After:
Summary by CodeRabbit
Style
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.