-
Notifications
You must be signed in to change notification settings - Fork 379
chore(charts): refactor how skeleton theme is applied#10348
chore(charts): refactor how skeleton theme is applied#10348dlabrecq merged 2 commits intopatternfly:mainfrom
Conversation
Refactored the skeleton theme to use a cleaner implementation in chart-theme.ts and chart-theme-types.ts. This ensures our custom component props (e.g., label, etc.) are not passed to Victory charts, avoiding potential type errors.
For Victory props and our custom components, there are...
- Two base themes;
BaseThemeandBaseComponentTheme. - Two color themes;
ColorThemeandColorComponentTheme - Two skeleton themes; one for Victory props, one for our custom components.
As before, the color theme is applied by merging base themes with either a skeleton or color theme (e.g., blue color theme props). If a custom component is being rendered (e.g., donut, bullet, etc.), component theme props are also merged with the base theme.
Skeleton label props are now applied separately, via a component theme, so the theme object provided to Victory charts only contains supported properties.
Closes #10346
|
Preview: https://patternfly-react-pr-10348.surge.sh A11y report: https://patternfly-react-pr-10348-a11y.surge.sh |
| * @beta | ||
| */ | ||
| export interface ChartComponentThemeDefinitionInterface { | ||
| axis?: VictoryThemeDefinition; |
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.
It's unexpected to me that type of each of these props is now VictoryThemeDefinition.
I guess it's because the types spread all the same props in this API doc.
So i guess that's not a comment that requires any action. just me documenting what I learned for maybe future me
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 particular object may contain any of the theme props defined by VictoryThemeDefinition. For example, our custom donut utilization component overrides props for both Victory pie and legend. However, a custom component could override even more Victory theme props here (e.g., tooltip), if necessary.
nicolethoen
left a comment
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.
Lots of previously exported functions and objects were deleted - any concerns products will break as a result? Could these changes go into a v6 branch if so?
These helpers are exported for use in our chart components. However, All public APIs are defined at the package level via That said, I've marked as @deprecated and will remove them in the v6 branch. |
|
Your changes have been released in:
Thanks for your contribution! |