-
Notifications
You must be signed in to change notification settings - Fork 3
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: Add time slider to interactive filters #816
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
a6b2912
to
e42c3fe
Compare
f69990c
to
24bbdaa
Compare
24bbdaa
to
7281a05
Compare
7281a05
to
b9bdbb0
Compare
b9bdbb0
to
2ebea50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious and did a pre-review :) Looks great.
app/configurator/interactive-filters/interactive-filters-config-state.tsx
Show resolved
Hide resolved
app/configurator/interactive-filters/interactive-filters-config-state.tsx
Outdated
Show resolved
Hide resolved
return true; | ||
|
||
default: | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why could we have the default case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll also need to probably improve this, the case here is that activeField could be a lot of things – areaLayer, segment, x, y, but also interactive filter and here I wanted to catch these cases in more clear way, for all other strings the default case if triggered. Do you have an idea how to handle this better?
app/configurator/interactive-filters/interactive-filters-config-options.tsx
Show resolved
Hide resolved
I think we could have
in a next PR, and we would hide the interactive filter behind a flag. The PR is already getting big ;) What do you think ? |
Thanks @ptbrowne for review! Yes, it's getting big, I'll try to clean up the stuff and I even think it would make sense to leave things like chart container size, animation, design of the slider also for another PR and just fix things here + hide the feature behind a feature flag. I think it would make sense to merge before new UI is implemented so we can avoid lots of conflicts 😨 |
Yep, makes sense 💯 |
2ebea50
to
01d5ff9
Compare
01d5ff9
to
63c8067
Compare
28d4ab3
to
25d41c4
Compare
app/configurator/interactive-filters/interactive-filters-configurator.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🙇
25d41c4
to
3d066f8
Compare
3d066f8
to
5beacc6
Compare
1be54d1
to
add5270
Compare
Contributes to #802.
This PR introduces a basic time slider which can be used to interactively filter a chart based on time dimension.
How to test
/en/browse/dataset/https%3A%2F%2Fenvironment.ld.admin.ch%2Ffoen%2Fubd0104prod%2F5?dataSource=Int&flag__timeslider=true
(notice theflag__timeslider
, feature is not available by default).Parameter
.