Skip to content
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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Jul 10, 2024

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

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas rusackas requested a review from geido July 10, 2024 16:02
@dosubot dosubot bot added the change:frontend Requires changing the frontend label Jul 10, 2024
@rusackas rusackas added the review:checkpoint Last PR reviewed during the daily review standup label Jul 10, 2024
@rusackas rusackas marked this pull request as draft July 10, 2024 16:13
@rusackas rusackas requested a review from rtexelm July 10, 2024 16:13
@michael-s-molina michael-s-molina removed the review:checkpoint Last PR reviewed during the daily review standup label Jul 11, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 15, 2024
@@ -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';
Copy link
Member Author

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.
Copy link
Member Author

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.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 16, 2024
@rusackas rusackas changed the title chore(AntD5): fixing up some component imports/exports chore(AntD5): touchup on component imports/exports, theming ListViewCard Jul 16, 2024
@rusackas rusackas marked this pull request as ready for review July 16, 2024 16:40
@dosubot dosubot bot added the global:theming Related to theming Superset label Jul 16, 2024
Comment on lines +31 to +32
// 'border-radius': `${theme.gridUnit}px`,
border: `1px solid ${theme.colors.grayscale.light2}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 'border-radius': `${theme.gridUnit}px`,
border: `1px solid ${theme.colors.grayscale.light2}`,

Copy link
Member Author

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!

superset-frontend/src/components/ListViewCard/index.tsx Outdated Show resolved Hide resolved
// Card:
// colorBgContainerbackground: theme.grayscale.light4
// }}>
<ConfigProvider theme={listViewCardTheme}>
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

@rusackas
Copy link
Member Author

@dosu-bot any idea why my tests are failing? I get this kind of error several times:

FAIL src/pages/DashboardList/DashboardList.test.jsx
  ● Test suite failed to run

    Domain `undefined` was not found.

      87 |
      [88](https://github.com/apache/superset/actions/runs/10097447336/job/27922326057?pr=29545#step:13:89) |   translate(input: string, ...args: unknown[]): string {
    > [89](https://github.com/apache/superset/actions/runs/10097447336/job/27922326057?pr=29545#step:13:90) |     return this.i18n.translate(input).fetch(...args);
         |                                       ^
      [90](https://github.com/apache/superset/actions/runs/10097447336/job/27922326057?pr=29545#step:13:91) |   }
      91 |
      92 |   translateWithNumber(key: string, ...args: unknown[]): string {

      at Jed.dcnpgettext (node_modules/jed/jed.js:249:15)
      at Chain.fetch (node_modules/jed/jed.js:148:20)
      at Translator.fetch [as translate] (packages/superset-ui-core/src/translation/Translator.ts:89:39)
      at translate (packages/superset-ui-core/src/translation/TranslatorSingleton.ts:67:24)
      at Object.<anonymous> (src/pages/DashboardList/index.tsx:73:35)
      at Object.require (src/pages/DashboardList/DashboardList.test.jsx:34:1)

css={{
width: theme.gridUnit * 10,
width: Math.trunc(theme.gridUnit * 62.5),
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend global:theming Related to theming Superset preset-io size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants