-
Notifications
You must be signed in to change notification settings - Fork 343
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(apply redesigned classes/Text)
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 | ✅ 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? |
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.
|
|
||
| export interface Props { | ||
| contentInsights?: ContentInsights; | ||
| isRedesignEnabled?: boolean; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is one of the example for reference -
| const isMetadataViewV2Feature = isFeatureEnabled(features, 'contentExplorer.metadataViewV2'); |
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.
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.
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.