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

[Maps] timeslider #96791

Closed
wants to merge 32 commits into from
Closed

[Maps] timeslider #96791

wants to merge 32 commits into from

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 12, 2021

Implements #27714, fixes #53246

Timeslider MVP

Screen Shot 2021-04-12 at 8 26 48 AM

This PR adds a timeslider to maps. Manipulating the time slider will set a new piece of redux state, timeslice, that will be used by layers to only show data for that timeslice. Sources that are aggregation based must re-fetch for the selected timeslice. Document based sources will have to re-fetch for the selected timeslice if they contain incomplete results. If the document source has complete data, then mapbox filtering will be used to only show documents in the timeslice on the client without any additional network traffic.

@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.14.0 labels Apr 12, 2021
@nreese nreese requested a review from a team as a code owner April 12, 2021 14:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@katefarrar katefarrar removed their assignment Apr 19, 2021
@katefarrar
Copy link
Contributor

please ignore that I assigned and then unassigned myself to this. I was just looking at it and accidentally clicked assign 🤦 😆

@markharwood
Copy link
Contributor

Just saw the demo video - nice work.
One small comment on the choice of left/right arrow icons - I'd normally associate the arrow with a vertical line e.g. >| with "go to end" not "advance one step".
These icon tags seem to agree : icons for "last" vs icons for "next"

@elizabetdev
Copy link
Contributor

One note of feedback is to consider an active state on the timeline button to indicate that it is active/shown

@mdefazio I'm considering adding an active state. The first step was to improve the toolbar buttons here: #96913 (comment).

These buttons had a lot of issues. Like, no hover states. Some buttons are not coming from EUI but from mapbox. So I tried to make both look similar. Now all have a hover state and similar styles.

Then the idea is to introduce an active state for these types of buttons. The "drawing layers" buttons are also going to need.

Also we have this open issue in EUI: elastic/eui#4730. Should we create a common toolbar button? 🤔

@markharwood
Copy link
Contributor

One other note which you may have also considered - these sorts of time scrubbers often include a small date viz drawn to scale e.g sparkline underneath the slider to allow users to see the peaks and troughs at various points. This allows users to jump directly to the periods of interest e.g. to the peak congestion and do so without scrolling through the less interesting stages trying to gauge growth from one frame to the next.

@elizabetdev
Copy link
Contributor

Thanks @markharwood for all your feedback,

We have different milestones. And in a future version, we considering adding a data viz. The first version is the basic one. You can find the milestones here: https://www.figma.com/proto/5QGj3sKULvEloOpVJ9YpmC/Maps-Time-Slider?page-id=31%3A277&node-id=102%3A72643&viewport=148%2C1557%2C0.5&scaling=min-zoom3

These icon tags seem to agree : icons for "last" vs icons for "next"

I'll take a look if there's a better option. What icon would you recommend?

@elizabetdev
Copy link
Contributor

I'm trying to figuring out why the EuiDualRange doesn't fill 100% of the width.

I run some experiments here: https://codesandbox.io/s/holy-rgb-rgjg2?file=/index.js.

As we can see the top timeslider gets the full width and the one from the bottom doesn't. The one from the bottom uses the values calculated from getTicks(min, max, interval):

Screenshot 2021-04-28 at 14 34 49

It seems that the numbers being used for each tick value are not EuiDualRange friendly.

@nreese, is there any possibility to use more friendly values?

@nreese
Copy link
Contributor Author

nreese commented Apr 28, 2021

is there any possibility to use more friendly values?

What do you mean by "more friendly values"? Can you provide some examples of what works better then the current ticks?

@elizabetdev
Copy link
Contributor

You can see my experiments here: https://codesandbox.io/s/holy-rgb-rgjg2?file=/index.js.

I also talked with @chandlerprall and he said is going to look into this use case.

@nreese
Copy link
Contributor Author

nreese commented Apr 29, 2021

@elasticmachine merge upstream

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

really great PR.

Most comments are pretty high level for now:

  • split-up timeslider in standalone UX component to make it more cut-and-pastable
  • can we keep syncLayerWithMb synchronous?
  • can we remove timeslice-awareness from sources?
  • worth optimizing prefetching for ES-sources?

More details in line.

one bug I think:

Adding a filter (e.g. by clicking on a tooltip value) and then changing the timeslice causes the filter to get removed from the app.

@@ -77,7 +78,7 @@ export interface ILayer {
ownsMbLayerId(mbLayerId: string): boolean;
ownsMbSourceId(mbSourceId: string): boolean;
canShowTooltip(): boolean;
syncLayerWithMB(mbMap: MbMap): void;
syncLayerWithMB(mbMap: MbMap, timeslice?: Timeslice): Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if there is a way we can avoid this. Making the sync-operation async causes this asynciness to cascade. It's added complexity in the rendering, and not 100% sure if we're introducing issues.

e.g. the async should be explicitly handled here

this._syncMbMapWithLayerList();
}
this.props.spatialFiltersLayer.syncLayerWithMB(this.state.mbMap);

Copy link
Contributor

Choose a reason for hiding this comment

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

a similar comment wrt adding timeslice as optional param. All the other state propagates with search-filters, timeslice being the exception.

Right now, this is because of supporting masking, so it makes sense.

But could we keep track of timeslice state separately? e.g. it's not the only piece of state that is async but needed in styling. E.g. styleMeta is managed async too, but read-out synchronously when styles update.

Copy link
Contributor

Choose a reason for hiding this comment

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

the main question here imho would be: Can we make timeslice not special, and treat it like other parts of the global state required by layers.

@@ -0,0 +1,42 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

proposing to refactor this according to the RFC.

Split up timeslider in a standalone react-component #98355, so it can be more portable and move it to maps/public/timeslider.

Then, create a wrapper (e..g connected_component like here) that integrates it with the redux-store.

return false;
}

if (prevMeta.timeslice !== undefined) {
Copy link
Contributor

@thomasneirynck thomasneirynck May 4, 2021

Choose a reason for hiding this comment

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

Wondering if we it's worth thinking about this slightly differently. The masking-optimization takes effects when users narrow the timeslice. This implicitly works because the timeslider initializers with the entire range selected. However, it stops working when a user narrows the slider, and then adds a filter. This newly filtered data will be bracketed to the timeslice, not the timerange.

Could we prefetch the data still?

Similar logic exits in blended-layer, which precedes its data fetch with a count. So if the timeslider is active, could/should we do something similar for document layers.

^ this will be more difficult for mvt layers, where we cannot do a prefetch-check in the same way.

return indexPattern.timeFieldName ? indexPattern.timeFieldName : null;
}

canMaskTimeslice(prevMeta: DataMeta, timeslice?: Timeslice): boolean {
Copy link
Contributor

@thomasneirynck thomasneirynck May 4, 2021

Choose a reason for hiding this comment

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

Is there a way we can move the majority of this check to the layer-level, and have sources indicate more something like 'canPrefetchData`?

^ fwiw - it's hard for me now to image how #94811 and this PR will connect cleanly.

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

It looks good. But there are a few things that we discussed that we need to clarify/implement.

1 - When we open the timeslider for the first time, the range should be set to the first time window instead of all the time extent. @kmartastic can you confirm this?

Timeslider@2x

2 - When the range slider uses days, I don't think we need hours, minutes, seconds, and milliseconds. @nreese do you think we can reduce this just to use hours and minutes?

Timeslider-time@2x

@kmartastic
Copy link
Contributor

1 - When we open the timeslider for the first time, the range should be set to the first time window instead of all the time extent. @kmartastic can you confirm this?

Yes. My opinion is that is the better UX.

@kmartastic
Copy link
Contributor

2 - When the range slider uses days, I don't think we need hours, minutes, seconds, and milliseconds. @nreese do you think we can reduce this just to use hours and minutes?

I like the idea of showing a simple summary at all times and high-precision while dragging the slider or during playback.

What do you think? @miukimiu @nreese

Example:
kepler_ex

@nreese
Copy link
Contributor Author

nreese commented May 5, 2021

When the range slider uses days, I don't think we need hours, minutes, seconds, and milliseconds. @nreese do you think we can reduce this just to use hours and minutes?

I still think the display needs to contain the complete details of what can be selected. Even when the slider ticks show days, the thumbs can select time down to the milliseconds. Users need to know if the thumbs are at "jan 1st 00:00:00" or at "jan 1st 05:30:00". Those two values are not the same and users need to see that difference. Maybe the precision can only be displayed while the user is dragging the control?

@kmartastic
Copy link
Contributor

@miukimiu
Are there other layouts of the elements on the slider control that we can consider to make the precise display of time better?
What if the timestamps are on either side of the control?
What if the timestamps take up the top-line with more spacing and we move keep the step controls to the right of the slider?

@elizabetdev
Copy link
Contributor

elizabetdev commented May 5, 2021

Users need to know if the thumbs are at "jan 1st 00:00:00" or at "jan 1st 05:30:00". Those two values are not the same and users need to see that difference. Maybe the precision can only be displayed while the user is dragging the control?

If we're keeping all the details is better to always show all numbers. Just to show the details while dragging would introduce other issues. Like the time on the right side moving to the right when the precision numbers from the left side appear.

@kmartastic I'll try your suggestions to better accommodate the time. 👍🏽

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
maps 773 781 +8

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 2.7MB 2.7MB +20.6KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
maps 62.1KB 62.2KB +74.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

showTicks={true}
min={this.state.min}
max={this.state.max}
step={1}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of step={1} shouldn't we be using instead step={this.state.range}? So that users could only move the thumbs to the ticks?

If they can move the thumbs to values in between ticks, this can happen:

Screen+Recording+2021-05-06+at+04.36+PM.mov

@nreese and @thompsongl my assumption is that the step should be step={this.state.range} but when I try that the code fails with:

Uncaught Error: The value of 1619740800000 is not included in the possible
sequence provided by the step of 86400000 
at EuiRangeTrack.validateValueIsInStep (range_track.js:84)

Copy link
Contributor

@thompsongl thompsongl May 6, 2021

Choose a reason for hiding this comment

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

I'll leave it up to the Maps folks to decide how/if they want to limit selected ranges, but I have been working through step locking when it comes to the draggable highlight.
Especially when the step ticks are as far apart as they are in that video, it's very easy to get into interactions that feel off. That is, a small drag either results in no movement or movement that feels extreme and out of sync with the cursor.
Still working on it, but I think it'll be debounced so that the step only increments when you've dragged 50% of the step range.

but when I try that the code fails with

Could be something with the getTicks function? I'd probably need to pull down the branch to see how this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't we be using instead step={this.state.range}? So that users could only move the thumbs to the ticks?

I don't think so. The ticks are not very granular. Users will want very precise control over the selected time slice, down to minute or second or even millisecond depending on the use case. I have seen analysts working with time slider tools like these in past jobs and the time slice they want is often based on the data. They may want to narrow the time slice to show just a single document and flip back and forth between steps to play the change over and over.

@nreese nreese mentioned this pull request May 10, 2021
@nreese
Copy link
Contributor Author

nreese commented May 10, 2021

Replacing with #99661.

@thomasneirynck found some edge causes with client-side masking around adding filters that are going to take some thought to iron out. Decided its best to get a shippable MVP merged without client-side masking. Client-side masking can be added in future iterations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] change equals check for mapbox filter expressions