Skip to content

Add index to legendItem interface#10436

Merged
etimberg merged 2 commits into
masterfrom
types/legendItem
Jun 22, 2022
Merged

Add index to legendItem interface#10436
etimberg merged 2 commits into
masterfrom
types/legendItem

Conversation

@LeeLenaleee

Copy link
Copy Markdown
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

github-actions Bot commented Jun 22, 2022

Copy link
Copy Markdown

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
Copy Markdown
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

kurkle commented Jun 22, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

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