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

Profile bookmarks - Add search #2037

Merged
merged 8 commits into from
Apr 11, 2024
Merged

Profile bookmarks - Add search #2037

merged 8 commits into from
Apr 11, 2024

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Apr 11, 2024

Description

This adds search to the profile bookmarks screen.

Testing Instructions

  1. Start the app with a user that has Plus or Patron active and a few bookmarks
  2. Go to profile
  3. Scroll down and check if you see a Bookmarks option
  4. Tap on Bookmarks
  5. Notice that the screen now has a search field
  6. Search for a bookmark or episode title
  7. Notice that search results are filtered based on searched term or display a no bookmark view

Screenshots or Screencast

search.mp4

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • 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 added this to the Future milestone Apr 11, 2024
@ashiagr ashiagr requested a review from a team as a code owner April 11, 2024 12:29
@ashiagr ashiagr requested review from MiSikora and removed request for a team April 11, 2024 12:29
Copy link
Contributor

@MiSikora MiSikora left a comment

Choose a reason for hiding this comment

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

Works as expected.


@OptIn(ExperimentalCoroutinesApi::class)
suspend fun getBookmarkSearchResultsFlow() = searchQueryFlow.flatMapLatest { searchTerm ->
if (searchTerm.isNotEmpty()) {
Copy link
Contributor

@MiSikora MiSikora Apr 11, 2024

Choose a reason for hiding this comment

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

It would be useful to trim() the searchTerm so things like My bookmark work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here you go: a115ab3

Comment on lines +195 to +198
id = if (state.bookmarks.size > 1) {
LR.string.bookmarks_plural
} else {
LR.string.bookmarks_singular
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is already in our app but shouldn't this be handles with plural strings and not regular strings? Otherwise it doesn't translate well to different languages that have different grammar rules for different quantities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know the full context but it could be due to GlotPress not supporting plurals yet. I'll leave it for now and we can discuss it internally.

@ashiagr ashiagr enabled auto-merge (squash) April 11, 2024 13:41
@ashiagr ashiagr force-pushed the task/profile-bookmarks-part4 branch from a115ab3 to afa6e29 Compare April 11, 2024 13:47
@ashiagr ashiagr merged commit 56867d3 into main Apr 11, 2024
10 of 13 checks passed
@ashiagr ashiagr deleted the task/profile-bookmarks-part4 branch April 11, 2024 13:53
@b-aaron
Copy link

b-aaron commented Apr 15, 2024

When sorting by Podcast & Episode, custom files don't get split up for each file, so all of them are just sorted by timestamp, jumbled together.

Don't really have time to open an Issue right now, so just commenting here.

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.

None yet

3 participants