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

refactor(pie): remove recompose and convert to hooks #1143

Merged
merged 4 commits into from
Oct 19, 2020
Merged

refactor(pie): remove recompose and convert to hooks #1143

merged 4 commits into from
Oct 19, 2020

Conversation

DevelopmentByDavid
Copy link
Contributor

@DevelopmentByDavid DevelopmentByDavid commented Sep 30, 2020

Addresses #884 in the pie package by removing recompose.

A few notes/questions:

  • I am not sure what to do with the enhance.js file. It seems unnecessary since there are now hooks, but since I am unfamiliar with the project I left it there for now pending your advice.
  • In PieLayout.js I combined two of the computed props. Color is now always recomputed along with arcs. I don't think this combination changes anything since they seemed interdependent before, but I thought I should bring this change to your attention.
  • The one test passes and stories look normal, unless I missed something.
  • All compose withPropsOnChange were converted to React.useMemo

Any other additions or changes I'd be happy to make, just let me know.

Thanks!

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

Awesome work and thank you so much! A couple of questions and one change, but it looks great.

packages/pie/src/PieSlice.js Outdated Show resolved Hide resolved
packages/pie/src/PieLegends.js Outdated Show resolved Hide resolved
packages/pie/src/Pie.js Outdated Show resolved Hide resolved
packages/pie/src/PieLayout.js Outdated Show resolved Hide resolved
@wyze
Copy link
Contributor

wyze commented Oct 10, 2020

Also, go ahead and delete enhance file since it is no longer used.

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

Couple minor things and should be good!

packages/pie/src/PieLegends.js Outdated Show resolved Hide resolved
role,
} = props

const theme = usePartialTheme()
Copy link
Contributor

Choose a reason for hiding this comment

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

We can pass a theme prop to the component, so we need to override the default theme with those values. You can see this in the custom tooltip story. The background of the tooltip should be black and it is white.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this. I also noticed that in props.js that theme isn't a required or optional prop listed in the propTypes object. I don't know if this was done intentionally, but I wanted to bring it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it is present on PieLegends, might as well make it available for the main components as well.

Copy link
Contributor Author

@DevelopmentByDavid DevelopmentByDavid Oct 17, 2020

Choose a reason for hiding this comment

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

Hmm, where do the prop types for the pie specific theme reside? I looked in nivo/packages/core/theming/propTypes.js (since that is where PieLegends gets the propTypes declaration from), but it doesn't have the tooltip property that I see is being used inside of the stories. It looks like theme.tooltip is only ever defined via prop types in PieTooltip.js. I could of course do something like:

import { themePropType } from '@nivo/core'
export const PiePropTypes = {
    // other prop types 
    theme: PropTypes.shape({ ...themePropType, tooltip: PropTypes.shape({}) })
}

However, I'm not sure if that encompasses all of the custom theming for the pie package.

Upon further inspection, it looks like in nivo/packages/core/themeing/defaultTheme.js there is a tooltip default prop defined, but it is not found in the propTypes file.

Maybe this is starting to get outside of the scope of this PR.

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the work put in here @DevelopmentByDavid!

@wyze wyze merged commit 8ccf936 into plouc:master Oct 19, 2020
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.

2 participants