-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
chore(AntD5): touchup on component imports/exports, theming ListViewCard #29545
Conversation
@@ -18,7 +18,7 @@ | |||
*/ | |||
import { ReactNode, ComponentType, ReactElement, FC } from 'react'; | |||
import { styled, useTheme } from '@superset-ui/core'; | |||
import { Skeleton, AntdCard } from 'src/components'; | |||
import { Skeleton, Card } from 'src/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.
Using the Superset card now 🎉
@@ -138,7 +137,8 @@ const actionButtonsStyle = theme => css` | |||
} | |||
`; | |||
|
|||
const StyledUndoRedoButton = styled(AntdButton)` | |||
const StyledUndoRedoButton = styled(Button)` | |||
// TODO: check if we need this. |
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.
Ugh... I don't know how much we need any of this. But that seems like an exploration for another PR. We should probably make the main Button component more robust (i.e. use props) rather than overriding styles here.
// 'border-radius': `${theme.gridUnit}px`, | ||
border: `1px solid ${theme.colors.grayscale.light2}`, |
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.
// 'border-radius': `${theme.gridUnit}px`, | |
border: `1px solid ${theme.colors.grayscale.light2}`, |
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.
Oh, actually, there's probably a token we should be using for this!
// Card: | ||
// colorBgContainerbackground: theme.grayscale.light4 | ||
// }}> | ||
<ConfigProvider theme={listViewCardTheme}> |
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 will always override the Card theme though. If we wanted a dark theme in the future, this would always be forced to supersetTheme.colors.grayscale.light5
. So I guess we go back to considering the theme in Superset to be the controller, so that supersetTheme.colors.grayscale.light5
will need to be changed to something that works on a dark theme, which begs the question "Will this work everywhere?". Hardly so, as there is no specification as of where light5 should be used in the design system I think.
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.
However, this is still better than custom styles as we will likely be targeting class names which should be more sensible to changes than theme configs.
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.
Agreed... using this config provider here is specifically to append/override styles for that instance of the Card component. Other Cards higher up in the React/DOM tree will be left alone.
I agree that light4
is going to be problematic, as sometimes that would actually be a dark color needed. Part of the theming SIP is to add "surface" colors (if I'm remembering the name right) and foreground/text colors, which would be more context-based and less shade-based. We'll have to replace these in a million places in the codebase, but doing it on AntD tokens will be a whole lot easier than doing it in CSS style overrides.
1bdbc8f
to
994250d
Compare
@dosu-bot any idea why my tests are failing? I get this kind of error several times:
|
994250d
to
72dbca1
Compare
css={{ | ||
width: theme.gridUnit * 10, | ||
width: Math.trunc(theme.gridUnit * 62.5), |
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.
Can we use full gridUnits and drop that Math.trunc?
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 already got merged (agian) elsewhere...
If you search (using vscode) for gridUnit\s*\*\s*\d+\.\d+
you'll see a whole swath of these that need cleanup. Seems like a good rainy-day PR, and (hopefully) a listing rule could be added.
c0a58ca
to
b4c554b
Compare
SUMMARY
This component simplifies some component imports/exports, making it so that ONLY the Superset components use the base AntD components. We should be consolidating on Superset-wrapped AntD components whenever/wherever possible and relevant.
Also, this styles the ListViewCard to inherit the AntD theme "tokens" for the "Card" and adds a pattern to override them when necessary (rather than getting into granular CSS, which adds complexity and might cause future upgrade problems).
It also adds an AntD theme provider in Storybook, so all components will automatically be handed the theme.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Tested these components in Storybook and compared to the public storybook on
master.
.ADDITIONAL INFORMATION