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: Allow multiple identical labels in charts #1019

Merged
merged 32 commits into from
Apr 13, 2023
Merged

Conversation

ptbrowne
Copy link
Collaborator

@ptbrowne ptbrowne commented Apr 6, 2023

@bprusinowski is the author of the PR.

Fixes #1015.

This PR changes the way we index observations from indexing by label (or abbreviation) to indexing by value (for hierarchical dimensions, passed in a form of an additional property: dimensionIri/__iri__) or label (or abbreviation).

Basically, we change
{ label_or_abbreviation: Observation } into
{ value_or_label_or_abbreviation: Observation }.

We needed to make this change to allow displaying values with the same labels, which is now a case with hierarchical dimensions (but in most places, we are able to distinguish them anyway, either by a color legend below a chart or visually on the map).

There are also smaller bug fixes / improvements included:

  • showing color legend when it makes sense for line chart (when there is more than one value)
  • showing map legend when number of values > 1
  • mouse interactions with overlapping shapes (map)
  • added text stroke to CircleLegend labels
  • added dataSet prop to PublishedConfiguratorState to fix loading hierarchies for ColorLegend

How to test

  1. Go to https://visualization-tool-git-fix-same-labelled-ixt1.vercel.app/en/v/QIhTScAJpRyN?dataSource=Int
  2. See that we show Jura twice.
  3. Copy and edit this visualization.
  4. Change chart type to map.
  5. See that both Jura regions are displayed on the map (and that they are colored correctly and that the tooltip interactions work).
  6. Compare with https://int.visualize.admin.ch/en/v/eDRM5d2-KeD6?dataSource=Int
  7. Follow steps 2-5 and see that the charts and interactions are broken.

@vercel
Copy link

vercel bot commented Apr 6, 2023

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

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 13, 2023 3:12pm

ptbrowne and others added 5 commits April 6, 2023 14:53
Would provide a way to differentiate observations client side
otherwise we only have the label and it is not sufficient
...to be able to retrieve observations based on IRI and not solely on label
@bprusinowski bprusinowski changed the title fix/same labelled fix: Allow multiple identical labels in charts Apr 11, 2023
...there are two rendering modes for base maps, overlaid or interleaved (see https://deck.gl/docs/get-started/using-with-map). Previously we used interleaved mode, which caused the problems with hovering over overlapping shapes (wrong element was highlighted). Setting the option to recommended, overlaid mode, fixes the tooltip issues (and seems to have no impact on overall performance / behavior of the map).
...which led to undefined dataset when fetching hierarchies in ColorLegend and thus wrong (missing) hierarchical division
@bprusinowski bprusinowski marked this pull request as ready for review April 13, 2023 13:10
@bprusinowski bprusinowski self-requested a review as a code owner April 13, 2023 13:10
Copy link
Collaborator Author

@ptbrowne ptbrowne left a comment

Choose a reason for hiding this comment

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

LGTM 👏 Very happy to see that the hook could be extracted, and tested, I was wondering about that while reviewing the commits. Thanks for also fixing the other problems !

- Interleaved should be there for data layer polygons to be under lakes / roads
- depthTest: false makes picking not buggy
Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

🗺️

@bprusinowski bprusinowski merged commit 9820273 into main Apr 13, 2023
@bprusinowski bprusinowski deleted the fix/same-labelled branch April 13, 2023 15:32
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.

Same labelled values across hierarchy
2 participants