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

Fix XY-Scatter Plots Colors - Rebased #2303

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

ada-x64
Copy link
Contributor

@ada-x64 ada-x64 commented Jul 10, 2023

This PR continues the work in #2258. It needed to be rebased and tweaked. @zxy994 your commits are preserved.

@finos finos deleted a comment from finos-cla-bot bot Jul 10, 2023
@texodus
Copy link
Member

texodus commented Jul 11, 2023

Thanks @zxy994 @ada-x64

For charts with only a split_by, the chart no longer colors scatter points individually (as discussed) - but it should use the default color theme color var(--d3fc-series) (which is a pale blue in the Pro theme), instead it seems to be picking the first distinct color from the distinct color series calculation:

Screenshot 2023-07-10 at 11 52 53 PM

add seriesColorFromField and refactor for readability;
use monocolored legends
@ada-x64 ada-x64 force-pushed the fix-xy-scatter-plots-rebased branch from 45ae176 to dc8ee09 Compare July 11, 2023 14:56
@ada-x64
Copy link
Contributor Author

ada-x64 commented Jul 11, 2023

@texodus Amended to use default colors. Also tweaked some code to be more readable.

@finos finos deleted a comment from finos-cla-bot bot Jul 11, 2023
@finos finos deleted a comment from finos-admin Jul 12, 2023
@finos finos deleted a comment from finos-cla-bot bot Jul 12, 2023
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @zxy994 @ada-x64! Looks good!

I think there are still a few areas this should be improved on in the future:

  • We definitely need to improve the legend situation when both split_by and color columns are specified - right now it only shows them symbol legend (and I'd argue if we're to choose one, it should be the color legend).
  • Mapping split_by to symbols isn't great to begin with - we should add a new column for symbol and make split_by a multi-chart ala "Treemap" and "Sunburst" charts.
  • It's disorienting when switching from split_by to non-split_by configs with a color column, as the color palette change seeming arbitrarily (because the columns are named differently).
  • In the future, we really want to see tests for all commits. As this PR has gone through 4 rounds of code review, we'll hold of on these but please aim to bring test coverage with all PRs.

@texodus texodus merged commit 2d495d9 into finos:master Jul 13, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants