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

Add Animation Controls widget #634

Merged
merged 19 commits into from
Jul 3, 2019

Conversation

elenatorro
Copy link
Contributor

Fix CartoDB/cartoframes#812

Add a simple Animation Controls widget and its VL Bridge connector

Screenshot 2019-06-28 at 14 31 21

@elenatorro elenatorro changed the base branch from master to develop June 28, 2019 12:34
@elenatorro elenatorro requested a review from rjimenezda June 28, 2019 12:34
Copy link
Contributor

@rjimenezda rjimenezda left a comment

Choose a reason for hiding this comment

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

Looks good, but I feel like the Bridge code could be extracted to a common place since so much is shared between this and the time series.

…t/as-animation-controls-widget.scss

Co-Authored-By: Román Jiménez <drrouman@gmail.com>
Copy link
Contributor

@rjimenezda rjimenezda left a comment

Choose a reason for hiding this comment

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

Couple more things I saw

elenatorro and others added 3 commits July 1, 2019 12:10
…t/as-animation-controls-widget.scss

Co-Authored-By: Román Jiménez <drrouman@gmail.com>
…ange-slider.tsx

Co-Authored-By: Román Jiménez <drrouman@gmail.com>
Co-Authored-By: Román Jiménez <drrouman@gmail.com>
@VictorVelarde
Copy link
Contributor

VictorVelarde commented Jul 1, 2019

Have we considered adding the timestep visually (over or under the range slider)? I think we should. With that piece, I think we cover a much higher number of use cases.

Some ideas we could use:

  • a boolean property to display it: 'none', 'top', 'bottom'
  • a function receiving a date, to control the formatting

What do you think?

@rjimenezda
Copy link
Contributor

I think the idea was to keep it simple. Having it on the scrubber makes it tricky to format (overflow on the sides). Also I don't know if the range sliders supports having it elsewhere.

We already have some things to format dates due to the as-time-series-widget (it receives a format as per d3-time-format).

@elenatorro
Copy link
Contributor Author

I like the idea, actually there're other things it'd be great to have, like showing the current value of the animation. As Román says, we idea is to keep it simple now and create all the widgets, see how they work together, and then improve them. It'd also be great if the design team could be involved, but I'd do that in the next iteration.

@VictorVelarde
Copy link
Contributor

@elenatorro @rjimenezda I was talking about 'current value of the animation' indeed 😄 , but it's clear that I didn't explain myself correctly.

I aggree with not adding it now, in this first version, but I think that displaying the date will be eventually required. Let's wait then.

@elenatorro elenatorro merged commit 71f587b into develop Jul 3, 2019
@elenatorro elenatorro deleted the feature/812-cartoframes-add-animation-widget branch July 3, 2019 12:11
@andy-esch
Copy link

I just wanted to chime in that I agree with @VictorVelarde and think it's important for the next iteration of this widget to have the current timestamp (or number if animating on a numerical field). This helps the person viewing the map and interacting with it to understand the when and where of the events in the data. I also love @rjimenezda's idea of having a formatting string so the user can control how the timestamp (or number) is shown on the map.

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.

4 participants