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

Sync profile info and menu #37 & #72 #91

Merged
merged 1 commit into from Mar 6, 2019

Conversation

@lmorchard
Copy link
Contributor

commented Feb 28, 2019

Working toward implementing the profile menu and info display. Running into some issues and looking for an image asset, but figured I'd get a PR started for general kibbitzing.

Only meant to tackle one of these, but the two seemed to naturally run into each other.

Fixes #37
Fixes #72

Testing and Review Notes

  • Profile info in upper right should be reactive to signing in and out of sync.
  • Clicking on the icon in upper right should make a menu appear, which is adaptive to sync sign-in status.

To Do

  • Add unit tests and get coverage back up
  • Wrap en-US strings for l10n
  • Handle in Issue #92 Figure out how to link to "Sign in to Sync" - just opening an about:sync URL is not allowed. Maybe needs a Sync API experiment addition?
  • Need to find a "pointer" image to add to the menu bubble so it's connected to the avatar visually
  • Handle in other issues (e.g.) Need to verify other menu items are linking to the right things
  • Ensure the avatar display is correct - my account has no avatar, so it's just the anonymous image.
  • Remove the dev hack commit that opens the management page on reload!

@lmorchard lmorchard requested a review from mozilla-lockwise/desktop-engineering as a code owner Feb 28, 2019

@ghost ghost assigned lmorchard Feb 28, 2019

@ghost ghost added the in progress label Feb 28, 2019

@linuxwolf

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Figure out how to link to "Sign in to Sync" - just opening an about:sync URL is not allowed. Maybe needs a Sync API experiment addition?

#61 has some details on this, but the "TL;DR" is we'll need an extra method on the sync API to make the right calls and open the page. For some reason, I thought we already had, but now seem to recall from IRL discussions it would be pushed until this issue was to get done (-:

@6a68

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@lmorchard The sync folks pointed me at gSync.openPrefs as the right way to open the about:preferences#sync page, which we would indeed need to call from a new Sync API function: https://searchfox.org/mozilla-central/rev/c21d6620d384dfb13ede6054015da05a6353b899/browser/base/content/browser-sync.js#656

See also the usage of openPrefs elsewhere in that file: https://searchfox.org/mozilla-central/rev/c21d6620d384dfb13ede6054015da05a6353b899/browser/base/content/browser-sync.js#273

I think we want to change the entryPoint value to something lockbox-specific. I've been told this will be passed to fxa server-side telemetry, but not firefox telemetry, so it'll help tell us something indirectly about lockbox usage, but I'm not sure where we'd put that in the metrics.md doc. As for addon telemetry, the metrics doc does mention reconnect_sync and signin_sync for clicks on this menu item.

@6a68

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

whoops! I gotta remember to refresh before leaving comments :-P

@linuxwolf

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

We ought to discuss/verify with @mozilla-lockbox/product ...

I believe (but seem to not have recorded herein) that handing some specific calls -- like opening the sync prefs a la #61 -- is deferred to those issues. The rationale being to divide the work into accomplishable chunks.

Personally, I would consider making making the individual menu items truly work to be a stretch goal for this PR, and should be captured in other issues already.

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Personally, I would consider making making the individual menu items truly work to be a stretch goal for this PR, and should be captured in other issues already.

Just noticed issue #92 - I'd be fine with deferring the menu items here to other issues. I'm kind of guessing on some of the URLs, and I assume there should / will be telemetry asks for them that I haven't implemented here.

@lmorchard lmorchard force-pushed the lmorchard:72-sync-profile-menu branch from 325767a to 2772130 Mar 4, 2019

@linuxwolf

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Just noticed issue #92 - I'd be fine with deferring the menu items here to other issues. I'm kind of guessing on some of the URLs, and I assume there should / will be telemetry asks for them that I haven't implemented here.

In your defense, some of the expectations/details weren't as spelled out as maybe they should have been before you started the work.

I'm walking through this PR to leave comments, hopefully it helps avoid some extra churn.

@linuxwolf
Copy link
Member

left a comment

🚜 drive-by 🚜

although still a WIP this is impressive; as far as I can tell it's just missing tests -- all the expected visuals and functionality are in place!

My only general comment regards the links to external assets (FAQ, Feedback): they are opening in a new window. @meandavejustice encountered this with doorhanger work, and resulted in the "open_site" background message, which I think also happily aids in future telemetry work?

More comments inline, and great work.

src/list/actions.js Outdated Show resolved Hide resolved
src/list/actions.js Outdated Show resolved Hide resolved
src/list/actions.js Outdated Show resolved Hide resolved
src/list/actions.js Outdated Show resolved Hide resolved

@lmorchard lmorchard force-pushed the lmorchard:72-sync-profile-menu branch from cab974b to 852529a Mar 5, 2019

Implement header UI and profile menu
- Logged-in profile and avatar shown when available

- Profile menu items and new strings for l10n

- Add listeners for sync profile data and state

- Move FAQ & feedback links to open in background script

- Header UI with Login / Monitor tabs (monitor content TBD)

- AppPanes component for switching views with tabs

Fixes #72
Fixes #37

@lmorchard lmorchard force-pushed the lmorchard:72-sync-profile-menu branch from a7a3b02 to 0e59833 Mar 6, 2019

@lmorchard lmorchard changed the title WIP: Sync profile info and menu #37 & #72 Sync profile info and menu #37 & #72 Mar 6, 2019

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Whew, okay - this should have unit tests now, except for some of the TODO bits. Also squashed commits and tidied up a bit, so I think this qualifies as no longer WIP (pending any further review notes)

@linuxwolf
Copy link
Member

left a comment

r+

very awesome!

@lmorchard lmorchard merged commit 69d9244 into mozilla-lockwise:master Mar 6, 2019

4 checks passed

WIP Ready for review
Details
codecov/patch 75.3% of diff hit (target 50%)
Details
codecov/project 93.6% (-1.4%) compared to cf9edbd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.