-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
refactor: Move ProgressBar to Antd #11875
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11875 +/- ##
===========================================
- Coverage 67.65% 55.11% -12.54%
===========================================
Files 927 421 -506
Lines 45009 14802 -30207
Branches 4306 3815 -491
===========================================
- Hits 30450 8158 -22292
+ Misses 14455 6644 -7811
+ Partials 104 0 -104
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
*/ | ||
import React from 'react'; | ||
import { styled } from '@superset-ui/core'; | ||
import { Progress as AntdProgress } from 'src/common/components'; |
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'd like to nudge superset devs toward using only this styled ProgressBar and not the one from Antd directly - just to keep things consistent. What do you think about importing from antd
directly here, and NOT export Progress
from common/components/index.tsx?
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.
... you could even import this component from common/components/index.tsx
and export it as Progress
- then people could import { Progress } from 'src/common/components'
as it if were the AntD one, but it'd really be this enhanced/styled one.
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 would rather go with your first suggestion and import the Antd Progress directly into the enhanced component. From src/common/components only export the enhanced Progress so that accessing the Antd one directly becomes unlikely/harder.
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.
This looks great... just had one question/suggestion about enforcing best practices by having only one export
for progress bars in the codebase (your enhanced one, rather than AntD's).
Really appreciate the storybook entry. Trying to fill in more of those!
SUMMARY
Moving the ProgressBar component from react-bootstrap to AntDesign and customizing it with Emotion/CSS to enable a striped background which is not supported by Antd by default.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TEST PLAN
ADDITIONAL INFORMATION