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

marimekko: all dimension input data is propagated #2681

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

Linschlager
Copy link
Contributor

Currently, only the dimensionId as well as the patched value function are propagated to the computed data.
In order to use custom colors per dimension, it is necessary to propagate the entire object to the computed one so the dimension data is still available.

Resolves #2677

Copy link

vercel bot commented Jan 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nivo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 21, 2025 0:33am

@plouc
Copy link
Owner

plouc commented Jan 20, 2025

Hi @Linschlager,

First of all, thank you for this contribution!

I'm not convinced that merging the raw dimension directly into the DimensionDatum is the best approach.
I think this could be confusing, as we could end up with some arbitrary properties in DimensionDatum, especial ly for users who don't use TypeScript.
It would probably be best to isolate, for example adding DimensionDatum.original or something similar, and maybe to use a generic for the dimensions if needed.
This approach would require using the colors property to get the color from DimensionDatum.original, but I think it would be cleaner.

Please let me know what you think.

@Linschlager
Copy link
Contributor Author

Linschlager commented Jan 21, 2025

Hi @Linschlager,

First of all, thank you for this contribution!

I'm not convinced that merging the raw dimension directly into the DimensionDatum is the best approach. I think this could be confusing, as we could end up with some arbitrary properties in DimensionDatum, especial ly for users who don't use TypeScript. It would probably be best to isolate, for example adding DimensionDatum.original or something similar, and maybe to use a generic for the dimensions if needed. This approach would require using the colors property to get the color from DimensionDatum.original, but I think it would be cleaner.

Please let me know what you think.

Thanks for the feedback, @plouc,

This approach just seemed like the quickest and easiest way to achieve my goal. I do agree, however, that it might cause unintended side effects by allowing arbitrary inputs this way.

I updated my PR to not merge the raw dimension in but in a separate field where the impact is much more contained.

I also updated the example to account for that. I think it's still reasonably nice to use.

@plouc plouc merged commit af22689 into plouc:master Jan 23, 2025
4 of 5 checks passed
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.

LGTM

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

Successfully merging this pull request may close these issues.

marimekko: cannot set custom color via datum
2 participants