feat(classification): add Blueprint rendering behind isRedesignEnabled flag#4447
Closed
Nefaris wants to merge 7 commits intobox:masterfrom
Closed
feat(classification): add Blueprint rendering behind isRedesignEnabled flag#4447Nefaris wants to merge 7 commits intobox:masterfrom
Nefaris wants to merge 7 commits intobox:masterfrom
Conversation
…dByAiDetails class in redesign
Nefaris
changed the title
Contributor
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Classification
participant SC as SecurityControls
participant SCI as SecurityControlsItem
UI->>UI: check isRedesignEnabled
alt redesign enabled
UI->>SC: render with isRedesignEnabled=true
SC->>SCI: pass isRedesignEnabled=true to items
SCI->>SCI: render Text-based item (no tooltip)
else legacy
UI->>SC: render with isRedesignEnabled=false
SC->>SCI: pass isRedesignEnabled=false to items
SCI->>SCI: render legacy item with tooltip/IconInfo
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/classification/Classification.stories.tsx (1)
81-85: Consider addingargTypesto the default export for better Storybook controls.Adding
argTypesforisRedesignEnabled,controlsFormat, etc., would let reviewers toggle props interactively in the Storybook UI without needing separate stories for every permutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/classification/Classification.stories.tsx` around lines 81 - 85, Add argTypes to the default export in Classification.stories.tsx so Storybook can control props like isRedesignEnabled and controlsFormat interactively; update the exported object (the default export containing title/component/decorators) to include an argTypes field that declares isRedesignEnabled as a boolean control and controlsFormat (and any other relevant props) as appropriate controls (e.g., select or text) so reviewers can toggle these props in the UI without creating separate stories.src/features/classification/security-controls/SecurityControlsItem.js (1)
14-17: Consider documenting thattooltipMessageis unused whenisRedesignEnabledis true.When the redesign path is active,
tooltipMessageis silently ignored. This is likely intentional, but a brief Flow/JSDoc comment on the prop or the type could help future maintainers understand this interaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/classification/security-controls/SecurityControlsItem.js` around lines 14 - 17, Add a brief Flow/JSDoc note to the Props type or above the SecurityControlsItem component explaining that the tooltipMessage prop is ignored when isRedesignEnabled is true; reference the Props type and the isRedesignEnabled and tooltipMessage symbols so maintainers know this interaction and why tooltipMessage may be unused in the redesign path.src/features/classification/Classification.js (1)
137-137: Semantic downgrade from<article>to<div>in redesign mode.The legacy path uses
<article>, which conveys that this is a self-contained composition. The redesign switches to<div>, which is semantically neutral. If this classification block is still a standalone content unit, keeping<article>(or using<section>) would be more accessible for screen readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/classification/Classification.js` at line 137, The Wrapper element currently uses a neutral 'div' when isRedesignEnabled is true, causing a semantic downgrade; update the ternary that defines Wrapper (const Wrapper = isRedesignEnabled ? 'div' : 'article') so the redesign path also renders a semantic container (e.g., 'article' or 'section') instead of 'div' — change isRedesignEnabled ? 'div' : 'article' to isRedesignEnabled ? 'article' : 'article' (or choose 'section' for grouping) so the classification block remains a standalone semantic element for accessibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/classification/security-controls/SecurityControls.js`:
- Around line 140-149: When isRedesignEnabled is true but shouldRenderLabel is
false there’s no trigger to open the modal even though securityControlsModal is
created; add a View All trigger for that case. Specifically, in the render
return (around itemsList and securityControlsModal) add a button (reuse
PlainButton with onClick={this.openModal} and content <FormattedMessage
{...messages.viewAll} />) or an equivalent trigger when isRedesignEnabled &&
!shouldRenderLabel so the modal (securityControlsModal) can be opened; keep the
existing legacy {!isRedesignEnabled && shouldShowSecurityControlsModal} branch
intact and ensure the new trigger follows the same visibility gating
(shouldShowSecurityControlsModal) and accessibility/text used by the existing
PlainButton.
---
Nitpick comments:
In `@src/features/classification/Classification.js`:
- Line 137: The Wrapper element currently uses a neutral 'div' when
isRedesignEnabled is true, causing a semantic downgrade; update the ternary that
defines Wrapper (const Wrapper = isRedesignEnabled ? 'div' : 'article') so the
redesign path also renders a semantic container (e.g., 'article' or 'section')
instead of 'div' — change isRedesignEnabled ? 'div' : 'article' to
isRedesignEnabled ? 'article' : 'article' (or choose 'section' for grouping) so
the classification block remains a standalone semantic element for
accessibility.
In `@src/features/classification/Classification.stories.tsx`:
- Around line 81-85: Add argTypes to the default export in
Classification.stories.tsx so Storybook can control props like isRedesignEnabled
and controlsFormat interactively; update the exported object (the default export
containing title/component/decorators) to include an argTypes field that
declares isRedesignEnabled as a boolean control and controlsFormat (and any
other relevant props) as appropriate controls (e.g., select or text) so
reviewers can toggle these props in the UI without creating separate stories.
In `@src/features/classification/security-controls/SecurityControlsItem.js`:
- Around line 14-17: Add a brief Flow/JSDoc note to the Props type or above the
SecurityControlsItem component explaining that the tooltipMessage prop is
ignored when isRedesignEnabled is true; reference the Props type and the
isRedesignEnabled and tooltipMessage symbols so maintainers know this
interaction and why tooltipMessage may be unused in the redesign path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/features/classification/Classification.js (1)
39-39:isRedesignEnabledis out of alphabetical order in thePropstype.The existing props follow alphabetical order (
isImportedClassification,isLoadingAppliedBy,isLoadingControls).isRedesignEnabledsorts afterisLoadingControlsand should be placed there (and mirrored in the destructuring).♻️ Proposed fix
isImportedClassification?: boolean, - isRedesignEnabled?: boolean, isLoadingAppliedBy?: boolean, isLoadingControls?: boolean, + isRedesignEnabled?: boolean,And in the component destructuring:
isImportedClassification = false, - isRedesignEnabled = false, isLoadingAppliedBy = false, isLoadingControls, + isRedesignEnabled = false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/classification/Classification.js` at line 39, Props type declaration has isRedesignEnabled out of alphabetical order and the component props destructuring must mirror the same order; move the isRedesignEnabled field in the Props/type declaration to follow isLoadingControls (i.e., place it after isLoadingControls and before any later props) and update the component's destructuring of props in Classification (where props like isImportedClassification, isLoadingAppliedBy, isLoadingControls are listed) to include isRedesignEnabled in the same alphabetical position so both type and destructuring match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/classification/Classification.js`:
- Around line 145-155: The redesign branch drops onClick and tooltip behavior:
when isRedesignEnabled is true, forward the onClick prop and tooltip/definition
handling to the Status usage (update the Status invocation in Classification.js
to accept onClick and to render the definition as a tooltip or accessible
fallback when messageStyle === "tooltip" or when isTooltipMessageEnabled is
true); add unit tests in Classification.test.tsx that exercise
isRedesignEnabled=true for (a) editable behavior via onClick, (b)
messageStyle="tooltip" showing the definition, and (c) color rendering; finally
verify or adapt the color values passed to Status (map legacy classification
colors to the design system tokens expected by Status or convert hex/variables)
to ensure compatibility.
---
Nitpick comments:
In `@src/features/classification/Classification.js`:
- Line 39: Props type declaration has isRedesignEnabled out of alphabetical
order and the component props destructuring must mirror the same order; move the
isRedesignEnabled field in the Props/type declaration to follow
isLoadingControls (i.e., place it after isLoadingControls and before any later
props) and update the component's destructuring of props in Classification
(where props like isImportedClassification, isLoadingAppliedBy,
isLoadingControls are listed) to include isRedesignEnabled in the same
alphabetical position so both type and destructuring match.
… and Classification
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/classification/Classification.js`:
- Line 148: The Status component usage passes text={name} where name is typed as
optional (name?: string) causing a Flow string | void error; mirror the legacy
path by casting name to string before passing it (e.g., use the same ((name:
any): string) cast pattern) so Status's text prop receives a string; update the
Status invocation in Classification.js to apply that Flow cast to the name
variable.
- Line 121: The AppliedByAiClassificationReason spacing is lost when
isRedesignEnabled is true because the class
'bdl-Classification-appliedByAiDetails' (which supplies margin-top:
var(--space-05) and margin-bottom: var(--space-5)) is removed; update the render
or CSS so the small top margin is preserved under redesign: either add a
redesign-specific class (e.g.,
'bdl-Classification-appliedByAiDetails--redesign') to
AppliedByAiClassificationReason when isRedesignEnabled, or adjust
.bdl-Classification--redesign to include the equivalent top spacing for that
child (maintain the space-05 gap for AppliedByAiClassificationReason) so visual
spacing remains consistent; reference the className usage where
isRedesignEnabled is checked and the AppliedByAiClassificationReason component
to apply the change.
---
Duplicate comments:
In `@src/features/classification/Classification.js`:
- Around line 146-156: The redesign branch is not forwarding the click handler
or tooltip like the legacy path: when isClassified && isRedesignEnabled the
<Status> render (using Shield, iconPosition and text={name}) should also accept
and forward the same props as <ClassifiedBadge> — specifically pass the onClick
handler and a tooltip prop (or tooltipText) populated from definition when
messageStyle/tooltip mode is requested; update the isRedesignEnabled branch to
forward onClick and the tooltip/definition (mirroring ClassifiedBadge's
tooltipText={isTooltipMessageEnabled ? definition : undefined}) so consumers
don't lose edit handlers or tooltip content.
Contributor
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Passes the
isRedesignEnabledfeature flag through theClassificationcomponent tree so child components can conditionally render Blueprint vs BUIE variants. This keeps the toggle controlled by the client app.Changes
isRedesignEnabledprop toClassification→SecurityControls→SecurityControlsItemUsage
Screenshots
Summary by CodeRabbit
New Features
Documentation
Style