feat(content-insights): add isRedesignEnabled flag for Blueprint button #4420
Conversation
|
|
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Sidebar as SidebarContentInsights
participant Summary as ContentInsightsSummary
participant Graphs as GraphCardPreviewsSummary
participant Metric as MetricSummary
participant Header as HeaderWithCount
participant Count as CompactCount
participant ButtonCmp as OpenContentInsightsButton
Sidebar->>Summary: render(isRedesignEnabled)
Summary->>Graphs: render(isRedesignEnabled)
Summary->>Metric: render(isRedesignEnabled)
Metric->>Header: render(isRedesignEnabled)
Header->>Count: render(isRedesignEnabled)
Summary->>ButtonCmp: render(isRedesignEnabled)
alt isRedesignEnabled == true
ButtonCmp->>ButtonCmp: useIntl() -> formatMessage()
Note right of ButtonCmp: render BlueprintButton and redesigned classes/Text
else isRedesignEnabled == false
Note right of ButtonCmp: render legacy Button and legacy span/Text paths
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Can we add some VRT tests here? |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/content-insights/OpenContentInsightsButton.tsx (1)
14-35: Correct the BlueprintButtonsizeprop value.The implementation correctly follows React hooks rules by calling
useIntlunconditionally at the component top. The early return pattern provides clean separation between the two rendering paths while maintaining consistentclassNamefor styling and test selectors.However, the
size="small"prop is incorrect. According to the@box/blueprint-webButton API, valid size values are'sm','default', or'lg'. Changesize="small"tosize="sm"on line 20. Thevariant="secondary"prop is correct.
|
|
||
| export interface Props { | ||
| contentInsights?: ContentInsights; | ||
| isRedesignEnabled?: boolean; |
There was a problem hiding this comment.
Instead of a prop isRedesignEnabled, can we change it to split or feature flag here which is better and easier to control and toggle?
There was a problem hiding this comment.
here is one of the example for reference -
There was a problem hiding this comment.
I'm not sure this approach would work well here, since the redesign should be controlled from the end-user app and enabled only within a specific scope, not globally. It is controlled by feature flag in eua.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/features/content-insights/CompactCount.tsx`:
- Line 24: CompactCount.tsx passes a redundant string into the classNames call
causing a leading space; remove the unnecessary `' '` argument so the JSX uses
classNames(className) (locate the className={classNames(' ', className)} usage
in the CompactCount component and replace it with a single-argument call to
classNames or equivalent handling of className).
🧹 Nitpick comments (4)
src/features/content-insights/messages.ts (1)
78-82: Inconsistent casing with other message defaults.
previewGraphTypenow uses title case ('Previews') while similar messages likedownloadGraphType('DOWNLOADS') andpeopleTitle('PEOPLE') remain in all caps. If the redesign requires title case, consider updating all related messages for consistency, or ensure the casing is handled via CSStext-transformto avoid divergence.src/features/content-insights/CompactCount.tsx (1)
34-37: ReuseformattedCountin the non-redesigned branch.
formattedCountis already computed at line 18 but the non-redesigned path callsformatCount(count, intl)again. Reuse the variable for consistency and to avoid redundant computation.Proposed fix
return ( <span className={classNames('CompactCount', className)} {...rest}> - {formatCount(count, intl)} + {formattedCount} </span> );src/features/content-insights/stories/tests/ContentInsightsSummary-visual.stories.tsx (1)
65-74: Consider adding aWithErrorRedesignstory.The error state is only tested without the redesign flag. For comprehensive VRT coverage, consider adding a story that tests the error state with
isRedesignEnabled={true}to ensure the error UI renders correctly in the redesigned layout.Suggested addition
export const WithErrorRedesign = { render: () => ( <Wrapper> <ContentInsightsSummary {...defaultProps} error={Object.assign(new Error('An error occurred'), { status: 500 })} isRedesignEnabled /> </Wrapper> ), };src/features/content-insights/MetricSummary.tsx (1)
3-4: Pre-existing: Shadowing of globalisFiniteandisNaN.Static analysis flags these imports as shadowing global names. While this is intentional (lodash versions handle edge cases like
isNaN(undefined)differently), consider aliasing them to avoid confusion:import isFiniteLodash from 'lodash/isFinite'; import isNaNLodash from 'lodash/isNaN';This is pre-existing code and not blocking for this PR.
4 participants
Passes the
isRedesignEnabledfeature flag through the Content Insights component tree so child components can conditionally render Blueprint vs BUIE variants. This keeps the toggle controlled by the end‑user app and enables gradual migration to Blueprint across the Content Insights UI.Changes
isRedesignEnabledprop toSidebarContentInsightsand pass it throughContentInsightsSummary→GraphCardPreviewsSummary→MetricSummary→HeaderWithCount/CompactCount/OpenContentInsightsButtonButtonandTextvariants when redesign is enabledContentInsightsSummarySuggestionForNewlyCreatedTemplateInstance) withwaitForUsage
Screenshot
Summary by CodeRabbit
New Features
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.