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

[Discover] Fixes state persistence after nav #5123

Conversation

ashwin-pc
Copy link
Member

@ashwin-pc ashwin-pc commented Sep 26, 2023

Description

replaces history api with application service's navigate method to correctly preserve state during redirection

Issues Resolved

closes #5056

Screenshot

Screen.Recording.2023-09-26.at.2.27.21.PM.mov

Testing the changes

Opening a saved search

  • Change timerange
  • Open a saved search
  • Timerange should persist

Creating a new search

  • Open a saved search
  • Change timerange
  • Click "New" in the menu bar
  • Timerange should persist

Reset

  • Open a saved search
  • Change timerange
  • Click "Reset" above the histogram
  • Timerange should persist

Check List

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

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #5123 (10b68c6) into feature/deangular (7a843e6) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@                Coverage Diff                 @@
##           feature/deangular    #5123   +/-   ##
==================================================
  Coverage              66.69%   66.69%           
==================================================
  Files                   3283     3283           
  Lines                  63059    63057    -2     
  Branches               10031    10031           
==================================================
  Hits                   42058    42058           
+ Misses                 18536    18534    -2     
  Partials                2465     2465           
Flag Coverage Δ
Linux_1 35.25% <ø> (ø)
Linux_2 55.23% <ø> (ø)
Linux_3 43.67% <0.00%> (+<0.01%) ⬆️
Linux_4 35.43% <ø> (ø)
Windows_1 35.27% <ø> (ø)
Windows_2 55.20% <ø> (ø)
Windows_3 43.67% <0.00%> (+<0.01%) ⬆️
Windows_4 35.43% <ø> (ø)

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

Files Coverage Δ
...plication/components/top_nav/open_search_panel.tsx 40.00% <0.00%> (+11.42%) ⬆️

@ashwin-pc ashwin-pc added discover for discover reinvent de-angular de-angularize work v2.11.0 labels Sep 26, 2023
@abbyhu2000
Copy link
Member

When i was testing, it seems like it still has some minor issues with the time filter:

  1. After i save a saved search, and go back to discover page, the time filter got reset to default; the time filter should be persisted.
  2. When i tried to load the previously saved saved search, time filter still did not get saved and remain as default.
Screen.Recording.2023-09-26.at.3.48.54.PM.mov

@ashwin-pc
Copy link
Member Author

Ah I see the issue. This is an issue with the breadcrumbs. Lemme fix that too :)

@ananzh
Copy link
Member

ananzh commented Sep 29, 2023

Regarding @abbyhu2000 's comment, should we fix it in another issue? or we want to fix it together here? @ashwin-pc

@ashwin-pc
Copy link
Member Author

Closed in favour of #5168

@ashwin-pc ashwin-pc closed this Oct 2, 2023
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.

3 participants