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 display of identicon on Connect page #8107

Merged
merged 1 commit into from
Feb 26, 2020
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Feb 26, 2020

The identicon was showing as a white circle on the connect page. This was a CSS error introduced when jazzicon was updated in #7898.

The white circle shown was the white border around the identicon. This circle is an adjacent div in the DOM, and was rendered underneath the identicon itself because it was placed first in the DOM.
Unfortunately the new version of jazzicon is no longer explicitly positioned (it used to have position: relative set internally), so now it's lower in the stack order regardless of DOM position.

Rather than placing the border adjacent and relying upon both elements being positioned, the border has been changed into a wrapping div instead. Now the stack order is more explicit.

Screenshots: Before:

old

After:

new

The identicon was showing as a white circle on the connect page. This
was a CSS error introduced when `jazzicon` was updated in #7898.

The white circle shown was the white border around the identicon. This
circle is an adjacent `div` in the DOM, and was rendered _underneath_
the identicon itself because it was placed first in the DOM.
Unfortunately the new version of `jazzicon` is no longer explicitly
positioned (it used to have `position: relative` set internally), so
now it's lower in the stack order regardless of DOM position.

Rather than placing the border adjacent and relying upon both elements
being positioned, the border has been changed into a wrapping `div`
instead. Now the stack order is more explicit.
@metamaskbot
Copy link
Collaborator

Builds ready [9b98767]
Page Load Metrics (665 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaintNaNNaNNaNNaNNaN
domContentLoaded339102466312058
load340102666512058
domInteractive339102366312058

}

.icon-with-fallback__identicon-border {
position: absolute;
Copy link
Member Author

Choose a reason for hiding this comment

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

I left alone the icon-with-fallback styles. I think it'd be nicer to update all of our icon borders to work this way as well, but that can come in a later PR.

@Gudahtt
Copy link
Member Author

Gudahtt commented Feb 26, 2020

cc @danfinlay , this could have also been fixed by adding position: relative to the jazzicon styles. Raphael had set position: relative internally on the svg element, so when Raphael was dropped for v2, the position was as well.

I think it's fine that jazzicon doesn't set the position anymore. The consumer can position it however they like. Just something to be aware of I guess.

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit 0a00408 into develop Feb 26, 2020
@Gudahtt Gudahtt deleted the fix-connect-identicon branch February 26, 2020 13:34
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