Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Migrate CollectionViewHolder to Compose #24333

Closed
gabrielluong opened this issue Mar 18, 2022 · 4 comments · Fixed by #24501
Closed

Migrate CollectionViewHolder to Compose #24333

gabrielluong opened this issue Mar 18, 2022 · 4 comments · Fixed by #24501
Assignees
Labels
compose Tickets involving Composable changes eng:health Improve code health eng:qa:verified QA Verified Feature:Collections Feature:HomeScreen
Milestone

Comments

@gabrielluong
Copy link
Member

gabrielluong commented Mar 18, 2022

Breakage of #22755

This refers to:

  • CollectionViewHolder showing an individual collection. (maybe instead of adding individual composables to the homescreen for each collection we could wrap them all in a list)

The compose versions can be added to the current RecyclerView but before doing so we should ensure there will be no performance regression introduced.
To profile the differences the steps from #21854 (comment) can be used.

┆Issue is synchronized with this Jira Task

@github-actions github-actions bot added the needs:triage Issue needs triage label Mar 18, 2022
@gabrielluong gabrielluong added eng:health Improve code health Feature:HomeScreen compose Tickets involving Composable changes and removed needs:triage Issue needs triage labels Mar 18, 2022
@Mugurell Mugurell self-assigned this Mar 22, 2022
@Mugurell
Copy link
Contributor

Migrating this to compose allows us to also simplify the current approach of adding having collections and collection items as separate viewholders on screen. (CollectionViewHolder plus one or more TabInCollectionViewHolder).

Since a collection contains a list of tabs the migrated to compose Collection can also be an expandable list of CollectionTabItems.
This would allow the expand/collapse animation be handled in the Collection composable and as such be smoother (and simpler), not having to sync the rounded corners like it's being done now (but buggy) between CollectionViewHolder and TabInCollectionViewHolder and also avoid checking whether one tab item is the same as another, a cumbersome check that is again buggy, all these seen in the following recording:

CollectionsIssues.mp4

cc @gabrielluong

@gabrielluong
Copy link
Member Author

gabrielluong commented Mar 28, 2022

I would prefer to see the migration to use a LazyColumn in #24316, which will display the list of collection items. I think we want an incremental approach just focus on the CollectionViewHolder which only contains the expandable header first to evaluate what is the extent of UI test failures and how do we coordinate with the release schedule and QA testing team to mitigate risk and fixing any breaking tests. Converting CollectionViewHolder will already include quite a bit of functionality - share, menu, expand. I am also anticipating other challenges like handling the existing selected border code, so I think there are plenty of things to cover here.

This is not to say we shouldn't convert everything to Compose, but to do it in reasonable steps so we can review the code more easily and evaluate the risk.

sarah541 added a commit to sarah541/fenix that referenced this issue Apr 5, 2022
@gabrielluong gabrielluong added this to the 102 milestone May 11, 2022
@mergify mergify bot closed this as completed in #24501 May 11, 2022
mergify bot pushed a commit that referenced this issue May 11, 2022
… created collection

The removal was initially scheduled to happen in the PR for #24333
but landed separately in #25045.
These are two small leftovers.
mergify bot pushed a commit that referenced this issue May 11, 2022
Doing just a file move in a separate commit will ensure git history of the file
is kept when this will be later updated.
mergify bot pushed a commit that referenced this issue May 11, 2022
After migrating to compose identifying widgets and interacting with them will
need to be updated.
Checked with Oana about the approach, commenting the code seems better.
@Mugurell Mugurell added the eng:qa:needed QA Needed label May 11, 2022
@Mugurell
Copy link
Contributor

@ QA: This was migrated to Compose but should look and behave the same as before.

@SoftVision-LorandJanos
Copy link

Verified as fixed with the latest Nightly 102.0a1 (2022-05-18T17:12:12.001240).
Devices used:

Samsung Galaxy Tab S3 (Android 9).
Google Pixel 4 (Android 12).

@SoftVision-LorandJanos SoftVision-LorandJanos added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compose Tickets involving Composable changes eng:health Improve code health eng:qa:verified QA Verified Feature:Collections Feature:HomeScreen
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants