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

feat: Show tree in MultiFilters #831

Merged
merged 27 commits into from
Nov 15, 2022
Merged

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Nov 9, 2022

Closes #800.

Design specs

  • Filter values in drawer are displayed as tree
  • Search in the tree keeps tree structure (e.g. when a child node is "found", all of its parents will also be displayed)

How to test

Look for a dataset with hierarchies (e.g. Bathing water quality (with Hierarchie) or Öffentliche Ausgaben TEST 3) and without hierarchies (e.g. Bathing water quality) and create a chart with color segmentation; afterwards edit filters to see if it works correctly.

@vercel
Copy link

vercel bot commented Nov 9, 2022

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

Name Status Preview Updated
visualization-tool ✅ Ready (Inspect) Visit Preview Nov 15, 2022 at 9:52AM (UTC)

@bprusinowski
Copy link
Collaborator Author

@ptbrowne I will take a look at the E2E test, but I am not sure if the error is related to changes from this PR (data loading error).

<FilterControls
selectAll={() => setPendingValues(flatOptions)}
selectNone={() => setPendingValues([])}
allKeysLength={flatOptions.length}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there should be a uniqueBy value here otherwise we can count twice values that are multiple times in the tree.

Copy link
Collaborator

@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 👍 Thanks, I have some remarks but I think they could also be tackled in a further PR.

@bprusinowski
Copy link
Collaborator Author

bprusinowski commented Nov 14, 2022

Thanks for the review @ptbrowne. For the first point, we include the color checkbox for all values, it's then visible or hidden based on hasValue property. I corrected this by checking if all values for a given depth level do not have a value; if so, the color box is not rendered. Otherwise, if there is at least one option with a value for a given depth level, it will be there (but not visible, just to align the text correctly).

The second point should also be ok after the changes.

Copy link
Collaborator

@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.

Not sure why the hierarchy values were not correctly handled before the final commit ?

LGTM 👏

app/rdf/queries.ts Outdated Show resolved Hide resolved
This is a temporary fix for something that seems like a bigger problem.

The symptom was that during the edition.spec tests, we'd have a data
loading error in place of the chart.

This is because inside the observationsQuery, one of the pattern
has an undefined subject.

The pattern is the one $cubeNode cube:observationSet ?observationSet0.
Here, $cubeNode is a NamedNode whose term should be the cube iri.

However here, the cube iri is undefined. The cube iri is taken from the
source, trying to find an outward edge with ns.view.cube and then
getting the term from the result.

The problem is that there is two edges from the source, one to the
old version of the cube, and one to the new version. This causes
the .term lookup here (https://github.com/zazuko/rdf-cube-view-query/blob/fac3834aadfdd3807ae96577ae12c9c5e865664e/lib/query/ViewQuery/Sources.js#L37)
to return undefined (because there are two links), so "terms" should be
used.

The problem goes away when updating the cube from the config (since
the old version is not loaded anymore), or when not using the cache
(using directly getRawCube instead of cachedGetRawCube). We still
do not know while not using the cache fixes the bad behavior.

TLDR: When using the cached cube loader, we cannot load cubes with an old
version: this seems to be because when we load several versions of a
cube, the source node ends up having two outward links (one to the
old version, one to the new version), instead of one outward link
to the right cube.
This is a temporary fix for something that seems like a bigger problem.

The symptom was that during the edition.spec tests, we'd have a data
loading error in place of the chart.

This is because inside the observationsQuery, one of the pattern
has an undefined subject.

The pattern is the one $cubeNode cube:observationSet ?observationSet0.
Here, $cubeNode is a NamedNode whose term should be the cube iri.

However here, the cube iri is undefined. The cube iri is taken from the
source, trying to find an outward edge with ns.view.cube and then
getting the term from the result.

The problem is that there is two edges from the source, one to the
old version of the cube, and one to the new version. This causes
the .term lookup here (https://github.com/zazuko/rdf-cube-view-query/blob/fac3834aadfdd3807ae96577ae12c9c5e865664e/lib/query/ViewQuery/Sources.js#L37)
to return undefined (because there are two links), so "terms" should be
used.

The problem goes away when updating the cube from the config (since
the old version is not loaded anymore), or when not using the cache
(using directly getRawCube instead of cachedGetRawCube). We still
do not know while not using the cache fixes the bad behavior.

TLDR: When using the cached cube loader, we cannot load cubes with an old
version: this seems to be because when we load several versions of a
cube, the source node ends up having two outward links (one to the
old version, one to the new version), instead of one outward link
to the right cube.

To fix this bug, I disabled the cube loader's cache. This means that
across request, we will not have a cache.
@bprusinowski
Copy link
Collaborator Author

@ptbrowne I guess it's related to versioning / unversioning of observations as for some datasets values didn't match and this resulted in many duplicated values e.g. in some cases there were "https://abc.org" in "proper" dimension values, and when using the previous query they had a format of "https://abc-test.org". I think it was the case for Ausgaben dataset.

@bprusinowski bprusinowski merged commit ba880b6 into main Nov 15, 2022
@bprusinowski bprusinowski deleted the feat/hierarchies-ui-refinements branch November 15, 2022 09:50
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.

Hierarchies UI Refinements
2 participants