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(charts): refactor how skeleton theme is applied #10348

Merged
merged 2 commits into from
May 14, 2024

Conversation

dlabrecq
Copy link
Member

@dlabrecq dlabrecq commented May 8, 2024

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; BaseTheme and BaseComponentTheme.
  • Two color themes; ColorTheme and ColorComponentTheme
  • 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

@patternfly-build
Copy link
Contributor

patternfly-build commented May 8, 2024

* @beta
*/
export interface ChartComponentThemeDefinitionInterface {
axis?: VictoryThemeDefinition;
Copy link
Contributor

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 😇

Copy link
Member Author

@dlabrecq dlabrecq May 8, 2024

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.

Copy link
Contributor

@nicolethoen nicolethoen left a 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?

@dlabrecq
Copy link
Member Author

dlabrecq commented May 9, 2024

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, ChartThemeTypes and react-charts/src/components/ChartTheme/themes/components/* are not exported at the package level and have been marked @private.

All public APIs are defined at the package level via react-charts/src/components/index.ts, while other helpers are intended for internal use and not supported.

That said, I've marked as @deprecated and will remove them in the v6 branch.

@dlabrecq dlabrecq merged commit 85f2be1 into patternfly:main May 14, 2024
13 checks passed
@patternfly-build
Copy link
Contributor

Your changes have been released in:

Thanks for your contribution! 🎉

@dlabrecq dlabrecq deleted the theme2 branch May 16, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chart - simplify skeleton theme
4 participants