Introduce variant prop for popover control item #4407
Conversation
Guria
requested review from
AmsterGet,
BlazarQSO,
maria-hambardzumian and
allaprischepa
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes refactor the way destructive actions are styled in popover menus across several components. Instead of assigning custom CSS classes for destructive actions, a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PopoverControl
participant MenuItem
User->>PopoverControl: Opens popover
PopoverControl->>MenuItem: Renders items with variant prop
alt variant is 'destructive'
MenuItem->>PopoverControl: Applies .popover-control__item-button--destructive class
else
MenuItem->>PopoverControl: Applies default styling
end
User->>MenuItem: Clicks item (e.g., delete)
MenuItem->>PopoverControl: Triggers onClick handler
Poem
✨ Finishing Touches🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/test-library #4407 +/- ##
=====================================================
Coverage 66.73% 66.73%
=====================================================
Files 85 85
Lines 995 995
Branches 142 142
=====================================================
Hits 664 664
Misses 300 300
Partials 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/pages/common/popoverControl/popoverControl.scss (1)
43-52: Check contrast for the new--destructivestateThe modifier correctly pins the text color to
var(--rp-ui-base-error)across all interactive states. Please verify that the default hover background (var(--rp-ui-base-bg-200)) still meets WCAG-AA contrast against this error color; if not, consider a dedicated background for the destructive variant to retain accessibility.Everything else looks consistent with the existing BEM naming.
app/src/pages/common/popoverControl/popoverControl.tsx (1)
26-34: Consider widening thevariantunion or making it exhaustiveRight now only
'destructive'is allowed. If you foresee additional visual intents (e.g. primary, warning, success) it would be better to encode them here to avoid future breaking changes:- variant?: 'destructive'; + variant?: 'destructive' | 'primary' | 'warning' | 'success';Keeping the union exhaustive helps TypeScript warn about unsupported variants early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/pages/common/popoverControl/popoverControl.scss(1 hunks)app/src/pages/common/popoverControl/popoverControl.tsx(2 hunks)app/src/pages/inside/productVersionPage/productVersionHeader/popoverTools/popoverTools.jsx(1 hunks)app/src/pages/inside/productVersionPage/productVersionHeader/popoverTools/popoverTools.scss(1 hunks)app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variable/variable.jsx(1 hunks)app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variable/variable.scss(1 hunks)app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variablesControl.jsx(1 hunks)app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variablesControl.scss(1 hunks)
🔇 Additional comments (6)
app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variable/variable.jsx (1)
57-60:variant: 'destructive'wired-in—ensure typings are updatedGood call moving to the semantic prop. Double-check that:
PopoverControl’s item type/prop interface includes'destructive'in its union so TS consumers compile cleanly.- No residual references to the removed red CSS class remain in the codebase.
Otherwise LGTM.
app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variable/variable.scss (1)
60-60: Nothing functionally changed in this hunk.app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variablesControl.scss (1)
60-60: Nothing functionally changed in this hunk.app/src/pages/inside/productVersionPage/productVersionHeader/popoverTools/popoverTools.scss (1)
23-23: Only removal of obsolete styles—no further action.app/src/pages/inside/productVersionPage/productVersionHeader/popoverTools/popoverTools.jsx (1)
108-111:variant: 'destructive'aligns with the new API – no issues spottedThe item correctly switches from ad-hoc CSS to the new semantic
variantprop.
No further action required.app/src/pages/inside/projectSettingsPageContainer/content/testLibrary/testData/variablesControl/variablesControl.jsx (1)
79-82: Proper adoption ofvariant: 'destructive'The destructive action now relies on the typed
variantinstead of a hard-coded CSS class — consistent with the refactor. Looks good.
Guria
requested review from
ViktorSoroka07,
OleksandrDragun,
Saveli0S and
yelyzavetakhokhlova
|
PR Checklist
developfor features/bugfixes, other if mentioned in the task)npm run lint) prior to submission? Enable the git hook on commit in your IDE to run it and format the code automatically.npm run build)?devnpm script)manage:translationsscript?Visuals
no changes in any visuals
@coderabbitai full review
Summary by CodeRabbit
New Features
Style
Refactor