fix(dashboard): use theme border for chart tiles - #41661
Conversation
Code Review Agent Run #fe93f0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
@YinkaMetrics can you add a before and after screenshot into the description |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41661 +/- ##
==========================================
- Coverage 64.61% 64.61% -0.01%
==========================================
Files 2684 2684
Lines 148514 148513 -1
Branches 34263 34262 -1
==========================================
- Hits 95969 95962 -7
- Misses 50786 50792 +6
Partials 1759 1759
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Updated the PR description with real before/after screenshots captured from the local Superset Sales Dashboard. Thanks for the prompt. |
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed. Scans N/A (trivial TS/Emotion diff, +22/-2). theme.colorBorder matches the precedent from #35199 and is already used elsewhere in this file (empty-droptarget border, fade-out box-shadow), resolves correctly in both light/dark via antd's alias-token algorithm. box-sizing: border-box prevents layout shift from the new border. The border-radius consolidation into the base rule is a reasonable, disclosed DRY cleanup (both .fade-in/.fade-out used the same value). Test asserts the real themed values via supersetTheme and follows the file's existing toHaveStyleRule/target pattern — not vacuous. No findings.
c12a9be to
81caebd
Compare
Code Review Agent Run #77c467Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
aminghadersohi
left a comment
There was a problem hiding this comment.
Re-review at new HEAD (author force-pushed a rewritten implementation). The new version routes through the existing opt-in dashboardTileBorder/dashboardTileBorderRadius override tokens instead of hardcoding colorBorder — an improvement over the prior version since it respects operator overrides. Two items:
superset-frontend/packages/superset-core/src/theme/types.ts:254documents these tokens' fallback as "no border" — this PR intentionally flips that default to a themed border (the fix for #41618) but doesn't update the now-stale comment. Not blocking, worth a one-line fix alongside.
See inline comment for the blocking item (failing required CI gate).
| width: 100%; | ||
| height: 100%; | ||
| background-color: ${theme.dashboardTileBg ?? theme.colorBgContainer}; | ||
| border: ${theme.dashboardTileBorder ?? |
There was a problem hiding this comment.
HIGH: the pre-commit (current) required check is failing at this HEAD — prettier-frontend reformats this file (files were modified by this hook). Run prettier --write (or the repo's pre-commit hook) locally and push the formatted version; this blocks merge as-is.
|
To resolve the failing pre-commit check, you need to run the project's Prettier formatting command locally. Since the npm run prettier -- --write superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsxAfter running this command, stage the changes and push them to your branch. This will ensure the file adheres to the project's style guidelines and allow the pre-commit check to pass. |
81caebd to
744d526
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #db62b3Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
aminghadersohi
left a comment
There was a problem hiding this comment.
Both prior findings addressed: border/border-radius are now single-line (79/80 chars, matches the file's existing style and prettier's 80-char default), and the types.ts:254 comment now correctly says "colorBorder border" instead of "no border". No new issues from a full re-scan.
On Bito's fade-out "regression" note: that's the intended behavior, not a bug — the whole point of this fix (#41618) is that the border should be consistent across all states (resting/fade-in/fade-out) rather than only appearing conditionally in .fade-out. Reverting that would reintroduce the original issue.
Note: CI hasn't actually executed at this HEAD yet (action_required — pending maintainer approval to run Actions for a first-time contributor's fork PR), so this isn't a green-CI confirmation, just a code-level check.
|
Thanks @YinkaMetrics, you got to the right approach first here (the |
|
Merged #41731, so I'll indeed close this after all, but let me know if you think there's more to address, and we can certainly reopen/revisit this. Thanks again! |
SUMMARY
Closes #41618
SCREENSHOTS
Captured from a local Superset Sales Dashboard with example data loaded.
Before:
After:
TESTING INSTRUCTIONS