[Unified Search] Fix date picker double click issue#218769
[Unified Search] Fix date picker double click issue#218769kowalczyk-krzysztof wants to merge 9 commits intoelastic:mainfrom
Conversation
|
|
||
| // ref.current.contains() is failing when the range is not in commonly used ranges | ||
| const refContainsTarget = | ||
| quickSelectButtonRef?.current?.getAttribute('itemid') === target?.getAttribute('itemid'); |
There was a problem hiding this comment.
Even though the HTML element is exactly the same, ref.current.contains() only works when the time range is one of the commonly used ones AND its label is visible. So an attribute check is needed.
| quickSelectButtonRef?.current?.getAttribute('itemid') === target?.getAttribute('itemid'); | ||
|
|
||
| // Check if the date range is in the commonly used ranges and prevent programmatic click | ||
| const isRangeInCommonlyUsed = commonlyUsedRanges.some(({ label }: { label: string }) => |
There was a problem hiding this comment.
For some reason, when time range is one of the commonly used ones and the label is visible (the time range can be both one of the common ones and display both from and to if you set it manually) a programmatic click causes a double click, which closes the popover.
The only reliable way of finding if the time range is both commonly used and displays the label I could find is checking the inner text of the label. This implementation works when you change the locale.
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
When manually going through the test case, the behavior works as expected, so it's a test only issue. |
davismcphee
left a comment
There was a problem hiding this comment.
Code-only review, Discover test changes LGTM.
| return await this.testSubjects.exists('unifiedHistogramChart'); | ||
| } | ||
|
|
||
| public async closeDatePickerPopover() { |
There was a problem hiding this comment.
Nit: Seems like this method would be better suited to a common FTR service like TimePickerPageObject since it's not really Discover specific.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Async chunks
History
|
There was a problem hiding this comment.
The UI is inconsistent based on the selected time range. When the time range is a commonly used time ranges, the quick select popover opens and remains open. When the time range is relative or absolute, the quick select popover opens and immediately closes. This inconsistency does not make sense and for users, its hard to tell why one works vs the other. Since the user clicked the "quick select button", I would expect the quick select popover to open and remain open regardless of the selected time range.
Yes the popover does close and reopens when the time isn't commonly used one, although it's not that visible. This is most likely because |
@kowalczyk-krzysztof sorry it took a bit to get back to you! I found 2 minor problems with the solution in this PR. The first one is that, because it checks for common ranges only, the programatic click won't happen for "custom" quick selections e.g. "Last 2 minutes" (those you can select in the very first row of the popover panel). The other problem is related to keyboard use: if focus is on the query input, and you hit the Tab key, you move to the data picker's quick button and the trigger clicks as well, moving focus into the panel, which might be not what you want. I can understand why we suggested the programatic click approach, because fixing it from the EUI side is not straight-forward, but I'm thinking it might be worth evaluating the effort it would take, if possible. It's very edge-casey, but it could be considered a bug. Did you consider other solutions like not using I would want to discuss this with the entire team, early next week, to try and offer the best possible solution. I hope timing is OK. |
|
@acstll
I didn't want to change the
That would be awesome, thank you! |
|
Thank you for the patience @kowalczyk-krzysztof I have created an issue in EUI, because we would like to try and tackle this from EUI side, and make it so that the quick select popover remains open while toggling the However, since we won't be able to prioritize this right away, we suggest you move forward with the fix in this PR. Then after elastic/eui#8689 is fixed, we could remove the fix. Regarding the flash issue described above:
one idea to temporarily mitigate it would be adding a Let me know what you think. |
Sounds good. Thanks a lot for taking care of this. I'll play with |
|
I'll close this PR as I think it's better to wait for a proper fix. |
Summary
This PR fixes an issue where you would need to click the date picker two times if it was collapsed because unified search bar was expanded.
Closes: #177053