fenix icon indicating copy to clipboard operation
fenix copied to clipboard

Published 20 hours ago • verified publisher icon mozilla-mobile

Closes #26116: use new viewholders on history screen

Open mavduevskiy opened this issue 2 years ago • 5 comments

Using all fancy new viewholders.

Main features: filtering of remote/local items inside HistoryDataSource, calculating headers to remove when items are being removed inside HistoryAdapter, tracking of the empty state in HistoryAdapter.

Comments on tests are especially welcomed.

Pull Request checklist

  • [x] Tests: This PR includes thorough tests or an explanation of why it does not
  • [ ] Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • [ ] Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

  • [x] QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #26116

mavduevskiy avatar Jul 20 '22 16:07 mavduevskiy

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

mergify[bot] avatar Jul 20 '22 21:07 mergify[bot]

@MozillaNoah Hey, thank you for taking your time! As a non native English speaker, really appreciate help with grammar/styling. Fixed!

mavduevskiy avatar Aug 04 '22 02:08 mavduevskiy

@rocketsroger Thanks for the review! I followed your comments, had question just about one thing.

Played around with strings and html tags, smth like <string name="history_sign_in_create_account"><u>Or create a Firefox account to start syncing</u></string> would be displayed as. But multiple translations already used the CDATA tag. Should I leave it as it is? Remove the tag from the default language only or also adjust the existing translations? Screenshot_20220808_102413

mavduevskiy avatar Aug 08 '22 17:08 mavduevskiy

As discussed let's try:

  1. passing around headers to remove to HistoryView
  2. simplify the function calculateTimeGroupsToRemove

rocketsroger avatar Aug 08 '22 19:08 rocketsroger

... Should I leave it as it is? Remove the tag from the default language only or also adjust the existing translations?

In this case I think we should update it. (by creating a new _2 string) Since we know a good solution that will avoid a function call every time.

rocketsroger avatar Aug 08 '22 20:08 rocketsroger

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

mergify[bot] avatar Aug 16 '22 18:08 mergify[bot]

Leaving this for time being. Redesign is still planned, but priorities are different for now.

mavduevskiy avatar Aug 19 '22 16:08 mavduevskiy