fix: improve mui tables styles#213
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNormalized typography and button padding across MUI table components, added a Less-based dotted border to editable-table action cells, changed delete-confirm Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/mui/editable-table/mui-table-editable.js`:
- Line 201: Swal.fire is being passed the Material-UI palette token "warning"
for confirmButtonColor which SweetAlert2 expects a concrete CSS color; update
the Swal.fire invocation (the confirmButtonColor option) to use a hex value
(e.g. "#DD6B55") instead of "warning" so the confirm button renders
correctly—locate the Swal.fire call in mui-table-editable (the
confirmButtonColor property) and replace the token string with the concrete
color.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7421a0f0-5420-43ca-b077-ee3cb56d6d1d
📒 Files selected for processing (4)
src/components/mui/editable-table/mui-table-editable.jssrc/components/mui/editable-table/mui-table-editable.module.lesssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/mui-table.js
| type: "warning", | ||
| showCancelButton: true, | ||
| confirmButtonColor: "#DD6B55", | ||
| confirmButtonColor: "warning", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the showConfirmDialog implementation
fd -i 'showconfirmdialog' src
# 2) Inspect implementation to confirm what `confirmButtonColor` is passed into
fd -i 'showconfirmdialog' src --exec sed -n '1,220p' {}
# 3) Find how `confirmButtonColor` is used across the codebase
rg -n -C3 '\bconfirmButtonColor\b|sweetalert|Swal' srcRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 14269
Use a concrete CSS color value instead of Material-UI palette token.
Line 201 passes "warning" to Swal.fire() (SweetAlert2), which expects a CSS color string, not a palette token. This will fail to render the button color correctly. Use a hex color like "#DD6B55" to match the pattern used consistently throughout the codebase (e.g., src/components/table-editable/EditableTable.js, src/components/mui/formik-inputs/additional-input/meta-field-values.js).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/editable-table/mui-table-editable.js` at line 201,
Swal.fire is being passed the Material-UI palette token "warning" for
confirmButtonColor which SweetAlert2 expects a concrete CSS color; update the
Swal.fire invocation (the confirmButtonColor option) to use a hex value (e.g.
"#DD6B55") instead of "warning" so the confirm button renders correctly—locate
the Swal.fire call in mui-table-editable (the confirmButtonColor property) and
replace the token string with the concrete color.
There was a problem hiding this comment.
@santipalenque, the fix here is to revert confirmButtonColor back to the hex string "#DD6B55". SweetAlert2's Swal.fire() (which showConfirmDialog wraps) expects a CSS color value — passing the MUI palette token "warning" will result in the confirm button rendering without any color styling.
- Revert
confirmButtonColorfrom"warning"to"#DD6B55"insrc/components/mui/editable-table/mui-table-editable.js
Failed to handle agent chat message. Please try again.
There was a problem hiding this comment.
This is not an issue, in fact it fixes button color. This si not a SweetAlert2's, it's a MUI Dialog. @santipalenque
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
priscila-moneo
force-pushed
the
fix/improve-table-styles
branch
from
218619f to
d2f861f
Compare
priscila-moneo
changed the title
There was a problem hiding this comment.
Review Summary
This PR mixes unrelated changes with the stated goal of "improving MUI table styles." Several changes introduce bugs or regressions that need to be addressed before merging.
🔴 Critical Issues
1. package.json — version downgraded from 5.0.5 → 5.0.0
This is a regression. Package versions should only increment, not decrease. This will cause consumers pinned to 5.0.5 to lose the update. Please bump to at least 5.0.6 (or 5.1.0 if semver minor bump is warranted for these style changes).
2. showConfirmDialog.js — React 18 createRoot support removed
The previous code handled both React 17 (ReactDOM.render) and React 18 (createRoot). The new code removes the React 18 path entirely, reverting to the deprecated ReactDOM.render only. This will produce a console error in React 18 and is incompatible with React 19 where ReactDOM.render is removed. Please restore the createRoot branch.
3. search-input.js — clear button visibility uses stale term prop instead of searchTerm state
endAdornment: term ? ( // ← uses prop, not stateAfter a user types into the field, searchTerm (state) updates immediately but term (prop) only updates after the parent calls onSearch and re-renders. This means the clear button will not appear while the user is typing and has not yet pressed Enter. Change term to searchTerm here.
4. dropdown-checkbox.js — deselecting "all" is silently swallowed
When "all" is the only selected item and the user clicks it again (to deselect everything), the logic falls through all branches without calling onChange:
selected.includes("all")→true!value.includes("all")→false(all was already selected)selected.length > 1→false(selected is[]after toggle, or just["all"])
The removed else branch previously handled this by emitting ["all"] again (or you could emit [] to clear). Either way, onChange must be called for this case. This was a behavioral regression introduced by removing the else clause.
🟡 Notable Concerns
5. summit-dropdown/index.js — handleClick loses null guard
Previously handleClick guarded with this.state?.summitValue?.value !== undefined. The new code calls this.props.onClick(this.state.summitValue.value) unconditionally. While isDisabled prevents the button click normally, any direct programmatic call to handleClick when summitValue is null will throw a TypeError. The guard was also useful defensible behavior.
6. package.json — unnecessary "dependencies": {}
Adding an empty "dependencies": {} key is harmless but unnecessary. All packages are already correctly placed under devDependencies and peerDependencies.
7. Large test deletion scope
Three separate test suites had tests removed:
search-input.test.js— all debounced behavior tests removed (along with the feature)summit-dropdown/__tests__/summit-dropdown.test.js— entire file deletedmui-formik-datepicker.test.js— "retains selected date after picker closes" test removed
Removing tests that covered real behavior without replacement lowers overall confidence. If the debounced feature was intentionally removed, please document that in the PR description. If the summit-dropdown tests were removed because the ref-based API changed, please add equivalent tests for the new behavior.
✅ Looks Good
fontWeight: "normal"normalization across table cells — consistent and correctpadding: 0on actionIconButtoncomponents — appropriateconfirmButtonColor: "warning"— valid since this uses MUI<Button color>, not SweetAlert2styles.dottedBorderLeftapplied to action column cells — correct visual distinctionmui-formik-datepicker.jsblur handling — thehandleBlurwithsetTouched(true, true)is the correct approach- Renaming
styles.module.less→mui-table-editable.module.less— better naming convention
priscila-moneo
force-pushed
the
fix/improve-table-styles
branch
from
d2f861f to
5d39920
Compare
2 participants
ref: https://app.clickup.com/t/86b8tt4df
Summary by CodeRabbit
Style
Tests