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

Add index to legendItem interface #10436

Merged
merged 2 commits into from Jun 22, 2022
Merged

Add index to legendItem interface #10436

merged 2 commits into from Jun 22, 2022

Conversation

LeeLenaleee
Copy link
Collaborator

resolves #10319

…rts. Make datasetIndex optional since the before named charts dont include it.
@LeeLenaleee LeeLenaleee added the type: types Typescript type changes label Jun 22, 2022
@LeeLenaleee LeeLenaleee added this to the Version 3.8.1 milestone Jun 22, 2022
@github-actions
Copy link

github-actions bot commented Jun 22, 2022

Size Change: +73 B (0%)

Total Size: 257 kB

Filename Size Change
dist/chart.esm.js 74.3 kB +31 B (0%)
dist/chart.js 94 kB +29 B (0%)
dist/chart.min.js 66.8 kB +13 B (0%)
ℹ️ View Unchanged
Filename Size
dist/chunks/helpers.segment.js 21 kB
dist/helpers.esm.js 1.23 kB

compressed-size-action

@LeeLenaleee
Copy link
Collaborator Author

I just noticed that I worked within the chart.js repo itself. I'm sorry.

Used that when using the web editor it would automatically create a branch in my fork since I had no rights to do it within the normal repo.

I will take more care next time that the changes are made withing my fork of the project so this does not happen again, sorry.

@kurkle
Copy link
Member

kurkle commented Jun 22, 2022

I just noticed that I worked within the chart.js repo itself. I'm sorry.

Used that when using the web editor it would automatically create a branch in my fork since I had no rights to do it within the normal repo.

I will take more care next time that the changes are made withing my fork of the project so this does not happen again, sorry.

It's ok, just remove the branches after merge to keep it clean..

@LeeLenaleee
Copy link
Collaborator Author

Test is failing since the legendItem should not be allowed to be created without a datasetIndex.

I think that the best and cleanest approach is to add a baseLegendItem interface and a labelLegendItem interface, then let the current legendItem and doughnutLegendItem interfaces extend the base both with their index/datasetIndex mandatory. So it does not become a breaking change.

Or do we want to keep the typings more simple and remove the test that datasetIndex needs to be mandatory?

etimberg
etimberg previously approved these changes Jun 22, 2022
@etimberg
Copy link
Member

hmmm, I think if we removed the mandatory datasetIndex everyone will have to check it, but it's more correct so I think removing the test is the best option here

@etimberg etimberg merged commit 0312697 into master Jun 22, 2022
@etimberg etimberg deleted the types/legendItem branch June 22, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: types Typescript type changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type definition of LegendItem does not correspond to actual data
3 participants