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

Search history: Navigation and Clear All #767

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Feb 10, 2023

Part of #752

Description

This adds

  • Navigation for search history item
  • Clear all header and a confirmation dialog with a search history threshold (size > 3).

Note: Ignore UI, it doesn't exactly match the specs.

Testing Instructions

  • Build a search history of different types (podcast/ folder/ search term) of at least 4 items
  • Tap on each search history item separately
  • ✅ Notice that the appropriate page is opened for podcast, folder and the search is re-triggered for the search term
  • Return to search history and tap on Clear All
  • ✅ Notice that a confirmation dialog is shown (count > search history threshold)
  • Tap on Delete button in the dialog
  • ✅ Notice that search history is cleared
  • Search another query
  • Tap on Clear All
  • ✅ Notice that search history is cleared (no dialog shown since count < threshold)

Screenshots or Screencast

device-2023-02-10-184938.mp4

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ashiagr ashiagr requested a review from a team as a code owner February 10, 2023 13:48
@ashiagr ashiagr mentioned this pull request Feb 10, 2023
14 tasks
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

Only question I had is why we limit the search history to 5 item? The UI strongly implies that there isn't any more search history (to my eyes at least) since the list only takes up part of the screen, and it feels a bit odd when I have more than 5 items in my history that deleting one seems to make a new one appear. Certainly not a blocker on merging this PR.

Video of what I'm talking about
Screen.Recording.2023-02-10.at.11.38.46.AM.mov

@ashiagr
Copy link
Contributor Author

ashiagr commented Feb 13, 2023

when I have more than 5 items in my history that deleting one seems to make a new one appear.

I have seen this behavior in other apps, but I see your point. I'll raise it during the design review and update it accordingly.
Thank you for all the reviews!

@ashiagr ashiagr merged commit b7bb0bf into feature/752-search-history Feb 13, 2023
@ashiagr ashiagr deleted the task/752-search-history-clear-all-and-navigation branch February 13, 2023 06:13
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.

2 participants