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

Partition chart should pull from theme and spec not config #1185

Closed
markov00 opened this issue Jun 3, 2021 · 3 comments · Fixed by #1402
Closed

Partition chart should pull from theme and spec not config #1185

markov00 opened this issue Jun 3, 2021 · 3 comments · Fixed by #1402
Assignees
Labels
bug Something isn't working good first issue Good for newcomers Impact:Medium :partition Partition/PieChart/Donut/Sunburst/Treemap chart related

Comments

@markov00
Copy link
Member

markov00 commented Jun 3, 2021

Describe the bug
The config.width and config.height in the partition chart, at least in the pie chart layout, are overwritten by the parentDimensions.

The first part where the width and height are overwritten is visible in the getShapeViewModel before merging the partial config.

The second place is in the partitionMultiGeometries where we compute the chartWidth and chartHeight as derived from the marginedWidth/Height (always coming from the parentDimensions). On the same function we should also check the innerWidth/Height that also comes from the parentDimensions

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/youthful-curran-egqkv?file=/src/App.tsx
  2. Try to use width, height in the config, instead of the currently proposed workaround with margin/outerSizeRatio
  3. The size of the pie will be always the size of the parent, no matter what value are used as width/height in the Config

Expected behaviour
We should respect the config.width and config.height parameters exposed by the Partition component. If we don't want that feature, we should remove them from the API.

Version:

  • Elastic Charts: 29.2.0

Errors in browser console
N/A

@markov00 markov00 added bug Something isn't working :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels Jun 3, 2021
@nickofthyme nickofthyme added the good first issue Good for newcomers label Jun 7, 2021
@ron-debajyoti
Copy link
Contributor

I would like to take this up.

@markov00
Copy link
Member Author

Hey @ron-debajyoti thanks again for your availability
I've chatted with @monfera about this issue and we decided that we should remove the config.width and config.height from the public API instead of letting the user specify these values.

If you are willing to work on It we are ok with that. You can probably do that by using Omit TS utility, but it's worth checking the alternatives.

@ron-debajyoti
Copy link
Contributor

Hey @ron-debajyoti thanks again for your availability
I've chatted with @monfera about this issue and we decided that we should remove the config.width and config.height from the public API instead of letting the user specify these values.

Hey @markov00 thanks for the update. In that case, let me try to take up other open issues.

@rshen91 rshen91 self-assigned this Sep 10, 2021
@rshen91 rshen91 assigned nickofthyme and unassigned rshen91 Sep 14, 2021
@nickofthyme nickofthyme changed the title Partition chart doesn't respect config width/height parameters Partition chart should pull from theme and spec not config Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers Impact:Medium :partition Partition/PieChart/Donut/Sunburst/Treemap chart related
Projects
None yet
4 participants