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

Fix connect hardware styling #8680

Merged
merged 4 commits into from
May 28, 2020
Merged

Fix connect hardware styling #8680

merged 4 commits into from
May 28, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented May 28, 2020

Fixes create/add new hardware account form styling, in both Chrome and Firefox.

  • Rename some components and classes
  • Create account tabs: Connect -> Hardware

Screenshots from Chrome, but Firefox also tested.

Before
After

@rekmarks rekmarks requested a review from a team as a code owner May 28, 2020 16:08
@@ -4,6 +4,12 @@
box-shadow: 0 0 7px 0 rgba(0,0,0,0.08);
z-index: 25;
height: unset;
overflow-y: scroll;
Copy link
Contributor

Choose a reason for hiding this comment

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

looking into see if there is a way to get the right behavior without doing this as we should just use the browser's default scrolling mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we experiment with removing the hardcoded height from new-external-account-form?

i get the same basic behavior you're going for if I remove these changes and do this:

image

Copy link
Member

Choose a reason for hiding this comment

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

We had intentionally hidden the scrollbar on Chrome. I think @rekmarks was suggesting we do the same for Firefox to make it consistent.

I too would prefer that we use the default browser scrollbar where possible. Though the case could be made to consider the popup UI as a special case. The scrollbar looks pretty bad in the popup UI on some platforms, and consumes the very limited space we have to work with.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's a good point though - this does seem like the wrong place for this rule 🤔

Copy link
Member Author

@rekmarks rekmarks May 28, 2020

Choose a reason for hiding this comment

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

I thought we hid the scrollbar in the popup as a matter of policy.

Ref: transaction history

Copy link
Contributor

Choose a reason for hiding this comment

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

you can apply scrollbar-width: none to popup, just don't mess with overflow rules because you end up with 22 places you have to restore the default behavior to reduce jank.

image

My main argument is that by adding overflow-y scroll to a div you're making that div scroll independently of the browser and this can result in poor performance.

@rekmarks rekmarks requested review from brad-decker and Gudahtt May 28, 2020 18:03
@rekmarks rekmarks merged commit 22699cb into develop May 28, 2020
@rekmarks rekmarks deleted the fix-connect-hardware-styling branch May 28, 2020 18:47
@metamaskbot
Copy link
Collaborator

Builds ready [95869ae]
Page Load Metrics (606 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint328143115
domContentLoaded34877360413464
load34977560613464
domInteractive34777260413464

Gudahtt added a commit that referenced this pull request Jun 1, 2020
* origin/develop: (689 commits)
  Implement asset page (#8696)
  fix crash on signature request (#8709)
  Fix accounts menu styling (#8707)
  Delete docs/porting_to_new_environment.md (#8704)
  Remove unused `getToErrorObject` parameters (#8705)
  hide connected-status on metamask ext (#8703)
  Stop adding permissions middleware to trusted connections (#8701)
  Use `send` state for send flow token (#8695)
  do not display extension id in connection modal (#8699)
  Fix tab content disappearing during scrolling on macOS Firefox (#8702)
  close details when button is pressed (#8694)
  Refactor token selectors (#8671)
  Update eth_accounts permission description (#8693)
  Extract selected token from token input (#8692)
  Fix propType for Home defaultHomeActiveTabName (#8683)
  Fix create account form styling (#8689)
  Remove unused `getSelectedTokenAssetImage` selector (#8691)
  Remove `getTxParams` (#8676)
  do not show account mismatch alert on details (#8678)
  Fix connect hardware styling (#8680)
  ...
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.

4 participants