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.

Sign up for GitHub

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

Jump to bottom

Definitions for Pie component #207

Merged
merged 5 commits into from
May 29, 2018
Merged

Definitions for Pie component #207

merged 5 commits into from
May 29, 2018

Conversation

Psvensso
Copy link
Contributor

@Psvensso Psvensso commented May 28, 2018 •
edited
Loading

closes #206
ref: #197

I had to include the INivoLegendItem. Not sure if its possible to re-use definition files from other packages. If so when creating the legend definitions we could/should perhaps refactor this.

plouc
plouc requested changes May 28, 2018
View reviewed changes
Copy link
Owner

@plouc plouc left a comment

Choose a reason for hiding this comment

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

@Psvensso, thank you very much for this, I just have a few feedbacks.

packages/nivo-pie/index.d.ts Outdated

export type SettingsGetterFunc = (dataSlize: IPieDataITem) => string;

export interface IPieProps {
Copy link
Owner

Choose a reason for hiding this comment

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

The theme & animation props are missing

Copy link
Owner

Choose a reason for hiding this comment

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

Also, colors & colorBy, it's not obvious as it's added by the enhancer.

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 was expecting this i just couldent follow the enchancer deeå enough to see the props added. I will have another go later.

packages/nivo-pie/index.d.ts Outdated

export interface IPieProps {
data: Array<IPieDataItem>;
sortByValue: bool;
Copy link
Owner

Choose a reason for hiding this comment

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

not required, default to false

Copy link
Owner

Choose a reason for hiding this comment

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

in fact, except for data, all is optional as it's non required/defined in default props

Copy link
Owner

Choose a reason for hiding this comment

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

What do you think of the approach used for bar (with Partial)? https://github.com/plouc/nivo/blob/master/packages/nivo-bar/index.d.ts

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 just saw that they were all .required by the ReactProps.
Partial is a fine technique by me if they are all optional.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, some properties are required, but because the definition targets exposed API, you'll have defaultProps applied, so we have to take propTypes & defaultProps in account when creating definitions.

packages/nivo-pie/index.d.ts Outdated

export type LegendDirection = 'row' | 'column';

export interface INivoLegendItem {
Copy link
Owner

Choose a reason for hiding this comment

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

I should be removed, as others packages do not use this form, and maybe Nivo should be removed too

@plouc plouc mentioned this pull request May 28, 2018
Closed
26 tasks
@plouc plouc added 🍰 pie @nivo/pie package typescript labels May 28, 2018
plouc
plouc requested changes May 29, 2018
View reviewed changes
packages/nivo-pie/index.d.ts Outdated
data: Array<IPieDataItem>;
sortByValue: bool;
export type PieProps = Partial<{
data: Array<PieDataItem>;
Copy link
Owner

Choose a reason for hiding this comment

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

data is required, that's why it was handled separately for Bar components

Psvensso
Psvensso commented May 29, 2018
View reviewed changes
packages/nivo-pie/index.d.ts
export type LegendDirection = 'row' | 'column';

export type Legend = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please review the optional props. in Legend. Is this correct?

Copy link
Owner

Choose a reason for hiding this comment

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

Seems ok

@plouc plouc merged commit 0def4c3 into plouc:master May 29, 2018
@martijnboers
Copy link

martijnboers commented May 31, 2018 •
edited by plouc
Loading

When using the code given in the nivo.rocks example I receive the following error:

Type '{ data: any; margin: { "top": number; "right": number; "bottom": number; "left": number; }; inner...' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<ResponsivePie> & Readonly<{ children?: ReactNode; ...'.
  Type '{ data: any; margin: { "top": number; "right": number; "bottom": number; "left": number; }; inner...' is not assignable to type 'Readonly<Data & Partial<{ sortByValue: boolean; innerRadius: number; padAngle: number; cornerRadi...'.
    Types of property 'legends' are incompatible.
      Type '{ "anchor": "bottom"; "direction": "row"; "translateY": number; "itemWidth": number; "itemHeight"...' is not assignable to type 'Legend[]'.
        Type '{ "anchor": "bottom"; "direction": "row"; "translateY": number; "itemWidth": number; "itemHeight"...' is not assignable to type 'Legend'.
          Property 'data' is missing in type '{ "anchor": "bottom"; "direction": "row"; "translateY": number; "itemWidth": number; "itemHeight"...'.

@plouc
Copy link
Owner

plouc commented May 31, 2018 •
edited
Loading

Sorry, didn't tried it yet.
Spotted few problems:

  • PieDataItem.value is string instead of number
  • The website example add tooltip={null}, it should be removed, it's a bug with my code generation function
  • Some of the theme properties aren't pure CSS properties, eg. textColor
  • translateX & translateY shouldn't be required for Legend type
  • neither data for Legend as it's automatically injected and do not require the user to provide it
  • motion related properties are missing: animate, 'motionStiffness&motionDamping`

Unfortunately, I'm in a middle of something for now (Pie improvements, start/end angle support, canvas…), so I won't be able to fix this now.

@plouc
Copy link
Owner

plouc commented May 31, 2018

I've created #215

@snyk-bot snyk-bot mentioned this pull request Mar 28, 2021
Open
@itsdangeroustogoalone itsdangeroustogoalone mentioned this pull request Jun 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plouc plouc plouc approved these changes

Assignees

@Psvensso Psvensso

Labels
🍰 pie @nivo/pie package typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add TypeScript definitions for Pie package
3 participants
@Psvensso @martijnboers @plouc