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

Cascading facets #271

Merged
merged 24 commits into from
Jan 27, 2022
Merged

Cascading facets #271

merged 24 commits into from
Jan 27, 2022

Conversation

ptbrowne
Copy link
Collaborator

@ptbrowne ptbrowne commented Jan 21, 2022

In this PR, the left filters are reworked cascading facets.

  • Ability to fetch unique values for a dimension taking into account filters
  • Order of the filters is used so that filters below have values corresponding to filters above
  • Ability to reorder the filters (UX is to be improved)
  • See how to manage adding new filters (right now a filter without value is not reorderable, you need to first choose a value, then reorder). (This is because to manage order, we rely on the order of keys in the filter object. When a filter has no value, it is not yet in the filters prop. Should we keep a temporary invalid state while reordering and then when the user has chosen a value, save it in the chart config ? To be decided...)
    • The first available value is chosen
  • Optional filters are not shown by default and can be added later on

In this PR we rely heavily on the fact that in javascript, objects should retain the order of the keys in insertion order. It should work for now but I think that filters should be transformed into an array for clarity.

@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch 2 times, most recently from 73b9dd3 to 87a87d8 Compare January 21, 2022 16:24
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 21, 2022 16:32 Inactive
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from 87a87d8 to 2471329 Compare January 21, 2022 16:55
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 21, 2022 16:55 Inactive
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from 2471329 to 890bc3f Compare January 25, 2022 13:36
@ptbrowne ptbrowne had a problem deploying to visualize-ad-feat-casca-lqy6pi January 25, 2022 13:36 Failure
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from 890bc3f to 9776579 Compare January 25, 2022 13:47
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 25, 2022 13:48 Inactive
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from 9776579 to 2814ac0 Compare January 25, 2022 14:24
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 25, 2022 14:24 Inactive
@ptbrowne ptbrowne had a problem deploying to visualize-ad-feat-casca-lqy6pi January 25, 2022 14:31 Failure
@ptbrowne ptbrowne had a problem deploying to visualize-ad-feat-casca-lqy6pi January 25, 2022 14:36 Failure
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from 3e42746 to 62e3324 Compare January 25, 2022 14:39
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 25, 2022 14:39 Inactive
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 25, 2022 15:13 Inactive
@ptbrowne ptbrowne requested a review from bprusinowski January 25, 2022 16:04
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 25, 2022 16:43 Inactive
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from d9ac986 to f598b78 Compare January 25, 2022 16:49
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 25, 2022 16:49 Inactive
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.

LGTM 👌

@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from bdd4805 to 6ed7a68 Compare January 26, 2022 16:15
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 26, 2022 16:15 Inactive
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 26, 2022 16:46 Inactive
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from d8649aa to 8db5138 Compare January 26, 2022 17:22
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 26, 2022 17:23 Inactive
@ptbrowne ptbrowne requested a review from bprusinowski January 26, 2022 17:31
@ptbrowne
Copy link
Collaborator Author

@bprusinowski I rebased and pushed new commits, I think you finished your last review at
af52a0d .

@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 26, 2022 17:34 Inactive
@bprusinowski
Copy link
Collaborator

Yes, I will review the changes tomorrow morning :) Thanks for letting me know!

@bprusinowski
Copy link
Collaborator

bprusinowski commented Jan 27, 2022

Apart from one comment about a drag n drop icon name, everything looks good!

Edit: in fact, I noticed one thing on the preview deployment, not sure if it's related to changes in this PR.

scrolling.bug.-.720p.mov

@ptbrowne ptbrowne had a problem deploying to visualize-ad-feat-casca-lqy6pi January 27, 2022 09:01 Failure
Styling for making panel content take all the space should only be
done for the configurator panel
@ptbrowne ptbrowne force-pushed the feat/cascading-facets branch from 0db4855 to aac02a6 Compare January 27, 2022 09:05
@ptbrowne ptbrowne temporarily deployed to visualize-ad-feat-casca-lqy6pi January 27, 2022 09:05 Inactive
@ptbrowne
Copy link
Collaborator Author

Thanks good catch, this was due to styling for the panel left being applied a bit too broadly, I moved the styles only to the PanelLeft in the chart configurator. Those styles are there for the content of the panel left to take all the space available.

@ptbrowne ptbrowne merged commit 1d1ee32 into main Jan 27, 2022
@ptbrowne ptbrowne deleted the feat/cascading-facets branch January 27, 2022 10:14
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.

2 participants