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

Resolve dataset options (can be scriptable) #121

Closed
wants to merge 1 commit into from
Closed

Resolve dataset options (can be scriptable) #121

wants to merge 1 commit into from

Conversation

kurkle
Copy link
Owner

@kurkle kurkle commented Sep 26, 2022

Couple of fixtures fail locally (no spriting)

TODO:

  • add tests

@kurkle kurkle added the enhancement New feature or request label Sep 26, 2022
@cloudflare-pages
Copy link

cloudflare-pages bot commented Sep 26, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4f2ee49
Status: ✅  Deploy successful!
Preview URL: https://b737a8c1.chartjs-chart-treemap.pages.dev
Branch Preview URL: https://options.chartjs-chart-treemap.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Sep 26, 2022

Size Change: -52 B (0%)

Total Size: 19.4 kB

Filename Size Change
dist/chartjs-chart-treemap.esm.js 7.09 kB -23 B (0%)
dist/chartjs-chart-treemap.js 7.32 kB -22 B (0%)
dist/chartjs-chart-treemap.min.js 5.02 kB -7 B (0%)

compressed-size-action

@kurkle
Copy link
Owner Author

kurkle commented Sep 26, 2022

Actually not going to add any tests at this point, because none of the options make sense as scriptable currently.

The caption options are resolved in 2 places (dataset and element), and the first one determines the space while the 2nd one does the drawing. The should be merged somehow. Same story with borderWidth.

So this would need further refactoring.

@stockiNail
Copy link
Collaborator

@kurkle I agree. Believe me that I thought several times about that (I don't like it). I was thinking to move the size of the rect calculation after element creation but the buildData should be reviewed completely.

@kurkle
Copy link
Owner Author

kurkle commented Sep 26, 2022

Agreed about the buildData. I think it should be merged with element creation, and the available child rect calculation moved to element.

Something like

  • determine main rect
  • parse and create elements on this level
  • recurse

So actually quite much the same as the buildData, expect createting elements on the fly.

Label that a breaking change and do it in a major version. Maybe start using data instead of tree

@stockiNail
Copy link
Collaborator

So actually quite much the same as the buildData, expect createting elements on the fly.

exactly!

Label that a breaking change and do it in a major version. Maybe start using data instead of tree

I'll open an issue (tomorrow), if you agree, in order to do not forget for version 3.0.

@stockiNail
Copy link
Collaborator

stockiNail commented Sep 26, 2022

Before closing my laptop, I have preferred to submit the issue (I have also created the milestone 3.0.0 and added "breaking change" label because missing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants