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 a new flag in timefilter service to disable time filter #8981

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ruanyl
Copy link
Member

@ruanyl ruanyl commented Nov 27, 2024

Description

Update query assistant UX:

  1. when query assistant is active(assistant input expanded), disable the time filter and do NOT add the time filter to the PPL query
  2. when query assistant is collapse, enable the time filter and add the time filter to PPL

Issues Resolved

Screenshot

Screen.Recording.2024-11-27.at.15.19.49.mov

Testing the changes

Changelog

  • feat: hide time filter when query assistant input is expanded

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.88%. Comparing base (539675e) to head (c8bc4bd).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...plugins/data/public/query/timefilter/timefilter.ts 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8981      +/-   ##
==========================================
- Coverage   60.88%   60.88%   -0.01%     
==========================================
  Files        3802     3802              
  Lines       91070    91084      +14     
  Branches    14374    14376       +2     
==========================================
+ Hits        55444    55452       +8     
- Misses      32085    32089       +4     
- Partials     3541     3543       +2     
Flag Coverage Δ
Linux_1 29.01% <0.00%> (-0.01%) ⬇️
Linux_2 56.38% <ø> (-0.01%) ⬇️
Linux_3 ?
Linux_4 29.00% <54.54%> (+<0.01%) ⬆️
Windows_1 29.03% <0.00%> (-0.01%) ⬇️
Windows_2 56.34% <ø> (-0.01%) ⬇️
Windows_3 37.90% <50.00%> (-0.01%) ⬇️
Windows_4 29.00% <54.54%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

how come? apologies little bit defensive about the super date picker render because it's a source of a number of bugs due to the different variations of state it will be in. does this also mean if i expand query assist in discover and navigate to dashboards is the timefilter disabled still?

@ruanyl
Copy link
Member Author

ruanyl commented Nov 28, 2024

how come? apologies little bit defensive about the super date picker render because it's a source of a number of bugs due to the different variations of state it will be in. does this also mean if i expand query assist in discover and navigate to dashboards is the timefilter disabled still?

Hey @kavilla, no worry, I totally understand your concern.

does this also mean if i expand query assist in discover and navigate to dashboards is the timefilter disabled still?

In this particular case(assistant toggle), the state is reset whenever the query assistant bar unmount. So the state won't persist when navigated to dashboards.

And back to the question why introducing a new API in timefilter service?
I've been thinking of keeping this state as a local state of query assistant, but the value needs to be read in several places:

  1. The time filter UI component in QueryEditorTopRow
  2. In the PPLSearchInterceptor to decide whether to add time filter to query or not

It ends up with spreading a local state to multiple component which is even a worse design in my opinion. So I decided to have it in a central place where all the components have access to. I may not be the discover profession, really appreciate if you have any suggestions :)

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

Successfully merging this pull request may close these issues.

3 participants