Skip to content
This repository has been archived by the owner on Apr 17, 2021. It is now read-only.

Closes #2988: Remove Pocket from NavigationOverlay #2994

Merged
merged 4 commits into from
Jun 29, 2020

Conversation

psymoon
Copy link
Contributor

@psymoon psymoon commented Jun 25, 2020

Checklist

  • Confirm the acceptance criteria is fully satisfied in the issue(s) this PR will close
  • Add thorough tests or an explanation of why it does not
  • Add a CHANGELOG entry if applicable
  • Add QA labels on the associated issue (not this PR; qa-ready or qa-notneeded)

Copy link
Contributor

@mcomella mcomella left a comment

Choose a reason for hiding this comment

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

The code makes sense – remove the model (the fetch) and the UI – but I have difficulty feeling confident in the change if the code is still around in the APK, just in case there's a reference we missed. Can you remove the classes (which shouldn't have any utility anymore, afaict)?

After that, lgtm. If I'm not around to do the second review pass, someone else should be able to double check this and I think we'd be good.

To verify the behavior, I loaded it up in the Android TV emulator and moved my cursor around. I was unable to verify the a11y changes because I do not have a device: please make sure this is tested and that QA has tested it as well! Thanks.

@psymoon psymoon requested a review from mcomella June 26, 2020 20:24
Copy link
Contributor

@mcomella mcomella left a comment

Choose a reason for hiding this comment

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

It looks like a few things changed that shouldn't have and there's an open question or two.

@psymoon psymoon force-pushed the remove-pocket branch 2 times, most recently from 9c45a9b to 1ace39c Compare June 26, 2020 22:43
@psymoon psymoon requested a review from mcomella June 26, 2020 23:31
Copy link
Contributor

@mcomella mcomella left a comment

Choose a reason for hiding this comment

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

Still concerned about the telemetry thing but otherwise, lgtm – I'll let you figure out the telemetry thing.


image
Amazing. My favorite kind of change.

@psymoon psymoon merged commit 53d6b3c into mozilla-mobile:master Jun 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants