-
Notifications
You must be signed in to change notification settings - Fork 344
feat(content-insights): add isRedesignEnabled flag for Blueprint button #4420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Kamil Piekos seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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\n(add redesigned classes/text components)
else isRedesignEnabled == false
Note right of ButtonCmp: render legacy Button\nand legacy span/Text paths
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| return ( | ||
| <Text | ||
| as="span" | ||
| variant="bodyDefaultSemibold" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we update the props to follow alphabetical order
| graphData, | ||
| isLoading, | ||
| previousPeriodCount, | ||
| onClick, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: alphabetical order
| <div className={classNames('HeaderWithCount', { 'HeaderWithCount--redesigned': isRedesignEnabled })}> | ||
| {isRedesignEnabled ? ( | ||
| <> | ||
| <Text as="span" variant="bodyDefaultSemibold" color="textOnLightSecondary"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: alphabetical order
| <Text | ||
| className="MetricSummary-periodCount MetricSummary-periodCount--redesigned" | ||
| variant="titleXLarge" | ||
| as="span" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: alphabetical order
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.