Conversation
1def2b1 to
1e12491
Compare
4764e2a to
7f45e0e
Compare
6a02826 to
912ea56
Compare
b86da27 to
3c1beff
Compare
Fix ts error Fix ref type error Extracted i18n strings Fixed number rounding Fixed missing i18n string Add loading state to range slider control output Remove unnecessary change Fix i18n errors Apply formatter to range slider tick labels
d80ef42 to
9fba1fd
Compare
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
ThomThomson
left a comment
There was a problem hiding this comment.
The components look great, and the chaining functionality is working really well! I've left a few nits, mostly around how and when the embeddable fetches data, and how / what it outputs.
One UX issue I discovered is that in cases where a control that comes earlier in the chain uses the same field as the range slider control. In that case, the filter outputted from the range slider overwrites the earlier filter, even though the one that comes earlier should take priority.
This happens because we are outputting the user's selections as filters directly rather than outputting the narrowest range by comparing the selections to the available min max. This is quite an edge case, and could be fixed in a follow-up.
src/plugins/controls/public/control_types/range_slider/range_slider_embeddable_factory.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| public isEditable = () => Promise.resolve(false); | ||
|
|
||
| public getDisplayName = () => 'Range slider'; |
There was a problem hiding this comment.
Nit: Should probably use i18n here.
There is also a description that @Heenawter added here for the popover that we can take advantage of!
| await this.fetchMinMax(); | ||
| } | ||
| this.setInitializationFinished(); | ||
| this.setupSubscriptions(); |
There was a problem hiding this comment.
I noticed a small bug here where the minMax is fetched twice on startup. Not exactly sure how to fix it, but I think it's caused here.
src/plugins/controls/public/control_types/range_slider/range_slider_embeddable.tsx
Outdated
Show resolved
Hide resolved
| distinctUntilChanged((a, b) => isEqual(a.value, b.value)), | ||
| skip(1) // skip the first input update because initial filters will be built by initialize. | ||
| ) | ||
| .subscribe(() => this.fetchMinMax()) |
There was a problem hiding this comment.
Is there a reason you're re-querying the min and max whenever the user makes a selection? This could potentially cause a lot of queries and slow down performance. If it's only in order to publish the filters, I would say maybe those two functions should be split up?
| new Error(RangeSliderStrings.errors.getDataViewNotFoundError(dataViewId)) | ||
| ); | ||
| } | ||
| this.updateOutput({ dataViews: [this.dataView] }); |
There was a problem hiding this comment.
Since the control group hierarchical chaining system is listening to any output changes Outputting the data views every time you get them could cause lower performance due to the diffing functions having to run more than usual.
I see that you're batching the updateOutput in your getMinMax function, so this line could potentially just be removed?
| } | ||
|
|
||
| if (timeRange) { | ||
| const timeFilter = this.dataService.timefilter.createFilter(dataView, timeRange); |
There was a problem hiding this comment.
Here and below, you're batching the timeFilter and the filterpill filters. Those can be individually turned on and off in #128090. In the code currently, turning off filters would also turn off the time range.
I think it should be pretty straightforward to split those out!
| private buildFilter = async () => { | ||
| const { value: [selectedMin, selectedMax] = ['', ''] } = this.getInput(); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
I wonder if all of the early returns under here could be combined into one?
Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
[Range slider] set min width for panel
andreadelrio
left a comment
There was a problem hiding this comment.
🎉 Design LGTM with one pending to-do in a follow-up: make the size prop available inside the range slider files. I would like to be able to do some design adjustments for the small size.
|
@andreadelrio, since the size prop is tracked as part of the control frame state, it might not be straightforward to get access to it from inside the range slider components. On top of that, the All that to say, it might be better to use a trick similar to what we did for the popover size, using an |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
| @include euiBreakpoint('m', 'l', 'xl') { | ||
| .rangeSlider__panelOverride { | ||
| min-width: $euiSizeXXL * 12; | ||
| } | ||
| } |
There was a problem hiding this comment.
@cqliu1 was just using the Cloud environment to run through the questions for the usability testing sessions and I came across this bug. The range popover panel gets cut off when it is on the right side.
Removing this media query should solve the issue.
There was a problem hiding this comment.
@cqliu1 But we would need to figure out how to handle the small controls after removing that media query
There was a problem hiding this comment.
The quickest suggestion I can think of is to increase the size of the small controls to at least 300px ($euiSizeM * 25). What do you think @ThomThomson ?
There was a problem hiding this comment.
And likely using EuiResizeObserver is a more appropriate fix (like Option List is doing).
There was a problem hiding this comment.
I wonder if this is a reason to use the EuiPopover with EuiResizeObserver rather than using the EuiInputPopover?
I would be hesitant to just increase the size of the small control, as it isn't tied to anything solid. I.e. if the title was set to inline and Average Ticket Price rather than AvgTicketPrice the problem would still happen even with the larger small size.
Either way, I think this should be addressed in a follow-up bugfix PR so that we can get this merged in today for FF!
There was a problem hiding this comment.
The reason why I'm using EuiInputPopover is bc there were focus issues with using the EuiPopover + EuiResizeObserver where you would click on the lower bound number field of the anchor but focus would jump to the range slider in the popover, but if that would solve the popover sizing issues, then it might be worth finding a fix for the focus stuff. I agree with Devon and can address follow up PR.
There was a problem hiding this comment.
++ on fixing in a follow-up PR.
|
@elasticmachine merge upstream |
ThomThomson
left a comment
There was a problem hiding this comment.
Tested this locally again, and all the behaviours including the hierarchical chaining system are looking great!
I opened two issues while reviewing this: One is an enhancement to the validation system for the range slider.
The other is unrelated to this PR, and is something that I will fix.
Everything LGTM!
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
Related to #120035.
This introduces the range slider control.
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers