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 sorts categories alphabetically #942

Merged
merged 3 commits into from
May 12, 2023

Conversation

jamie23
Copy link
Contributor

@jamie23 jamie23 commented May 8, 2023

Description

Fixing an issue where the category list inside discover is sorted based on the english strings. If a user has their device in a different language then the list is not sorted alphabetically.

Fixes #673

Testing Instructions

  1. Fresh install of the app.
  2. Start the app.
  3. Navigate to the discover page with system settings in English.
  4. Notice that the categories listed under "Browse By Category" are sorted alphabetically.
  5. Set your system language to another language e.g. French.
  6. Navigate to the discover page.
  7. Notice that the categories listed under "Bowser By Category" are still sorted alphabetically.

Screenshots or Screencast

PocketcaseEng
PocketcastsFr

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

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

@jamie23 jamie23 requested a review from a team as a code owner May 8, 2023 14:04
@CLAassistant
Copy link

CLAassistant commented May 8, 2023

CLA assistant check
All committers have signed the CLA.

@jamie23
Copy link
Contributor Author

jamie23 commented May 8, 2023

Can you let me know if you want me to update the CHANGELOG.md file? It's my first contribution to the project so I wasn't sure if small fixes like this were necessary to note in that file.

Also, I tried to keep the changes as minimal as possible since it is my first contribution. By sorting based on the translation before passing to the adapter we still do another translation on the other side inside CategoriesListRowAdapter, we could get rid of the double translation by mapping the categories to a translated version before sorting, and passing this into the adapter. e.g.

DiscoverAdapter.kt:

onSuccess = { categories ->
    val sortedCategories = categories.map { it.copy(name = it.name.tryToLocalise(resources)) }.sortedBy { it.name }
    
    adapter.submitList(sortedCategories) {
        onRestoreInstanceState(holder)
    }
}

CategoriesListRowAdapter.kt:

override fun onBindViewHolder(holder: CategoryViewHolder, position: Int) {
    val category = getItem(position)
    holder.binding.lblTitle.text = category.name
    holder.binding.imageView.load(category.icon)
    holder.itemView.setOnClickListener { onPodcastListClick(category) }
}

If you prefer this, let me know and I can make the updates.

@ashiagr
Copy link
Contributor

ashiagr commented May 9, 2023

Hey @jamie23 👋

I tried to keep the changes as minimal as possible since it is my first contribution. By sorting based on the translation before passing to the adapter we still do another translation on the other side inside CategoriesListRowAdapter, we could get rid of the double translation by mapping the categories to a translated version before sorting, and passing this into the adapter.

Could you please go ahead and make this change to prevent double translation? It's your first contribution but you're spot on! 💪

Can you let me know if you want me to update the CHANGELOG.md file? It's my first contribution to the project so I wasn't sure if small fixes like this were necessary to note in that file.

Yes, please. Go ahead and add it to the change log as it is a user-facing change and it's good to highlight it.

Thank you! 🙇

@jamie23 jamie23 force-pushed the fix/sort-discover-categories branch from d7fbc6b to 190ee4f Compare May 10, 2023 23:11
@jamie23
Copy link
Contributor Author

jamie23 commented May 10, 2023

Thanks @ashiagr , I've made the changes both to the code and CHANGELOG.md, if there is anything else please let me know.

@ashiagr
Copy link
Contributor

ashiagr commented May 12, 2023

Looks great, thank you!

@ashiagr ashiagr merged commit 68f19c9 into Automattic:main May 12, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Categories only in alphabetized order in English
3 participants