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

Connected indicator info popup #8293

Merged
merged 8 commits into from
Apr 22, 2020
Merged

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Apr 4, 2020

This PR adds the popover containing info on the connected status indicator. It is intended to be shown to the user the first time they see it, but no more after that. Figma: https://www.figma.com/file/CvD7nhQVeSEB1QuSy3XhtY/oCap-%2F-Login-Per-Site?node-id=952%3A512

Demo video: https://streamable.com/n5k7kd

@metamaskbot
Copy link
Collaborator

Builds ready [c85a390]
Page Load Metrics (659 ± 54 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint35534152
domContentLoaded35080365811354
load35180465911354
domInteractive34980365711354

@danjm danjm force-pushed the connected-indicator-info-popup branch 3 times, most recently from fa1501d to 4c01f56 Compare April 4, 2020 01:00
@Gudahtt
Copy link
Member

Gudahtt commented Apr 4, 2020

Note that we only want to show this to existing users, not new users, so this should default to "shown". We'll have to add a migration to set it to "false", so existing users see it once.

@danjm danjm force-pushed the connected-indicator-info-popup branch from 4c01f56 to d056126 Compare April 4, 2020 15:55
@danjm
Copy link
Contributor Author

danjm commented Apr 4, 2020

@Gudahtt Migration added in d056126

@danjm danjm force-pushed the connected-indicator-info-popup branch from a5917d9 to 093402d Compare April 7, 2020 16:11
@metamaskbot
Copy link
Collaborator

Builds ready [093402d]
Page Load Metrics (645 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint37514242
domContentLoaded39689964412459
load39790164512459
domInteractive39689964312459

@danjm danjm force-pushed the connected-indicator-info-popup branch from cd231a3 to 16e4d44 Compare April 7, 2020 19:09
ui/app/components/ui/popover/popover.component.js Outdated Show resolved Hide resolved
ui/app/pages/home/home.component.js Outdated Show resolved Hide resolved
ui/app/store/actions.js Outdated Show resolved Hide resolved
ui/app/pages/home/home.component.js Outdated Show resolved Hide resolved
ui/app/pages/home/index.scss Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [593fdff]
Page Load Metrics (801 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaintNaNNaNNaNNaNNaN
domContentLoaded335113179818589
load341114080118589
domInteractive335113179818589

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

I found a few more minor things. I haven't tested this locally yet to see how it looks, but I'll do that tomorrow.

ui/app/components/ui/popover/index.scss Outdated Show resolved Hide resolved
app/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/app/components/ui/popover/index.scss Outdated Show resolved Hide resolved
ui/app/pages/home/index.scss Outdated Show resolved Hide resolved
@danjm
Copy link
Contributor Author

danjm commented Apr 20, 2020

All comments addressed.

Latest screen capture:

Peek 2020-04-20 10-25

@metamaskbot
Copy link
Collaborator

Builds ready [9bfe721]
Page Load Metrics (610 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint33513742
domContentLoaded32575960910048
load32676061010048
domInteractive32575860810048

Gudahtt
Gudahtt previously approved these changes Apr 20, 2020
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

button {
height: 39px;
width: 133px;
border-radius: 39px;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It looks a bit strange to have just this button rounded, while the rest are still rectangular. I know this matches the design, but there's a separate ticket to update the button designs across the entire app, and that would seem like the better time to make the buttons rounded rather than doing it in just this one place.

@danjm danjm force-pushed the connected-indicator-info-popup branch from 9bfe721 to e3735f8 Compare April 22, 2020 16:46
@metamaskbot
Copy link
Collaborator

Builds ready [e3735f8]

@danjm danjm merged commit 01985b2 into develop Apr 22, 2020
@danjm danjm deleted the connected-indicator-info-popup branch April 22, 2020 17:11
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.

3 participants