-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report: support narrow screens #6857
Conversation
@@ -118,6 +118,8 @@ class ReportUIFeatures { | |||
onMediaQueryChange(mql) { | |||
const root = this._dom.find('.lh-root', this._document); | |||
root.classList.toggle('lh-narrow', mql.matches); | |||
// Reset animations for the narrow view header height change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -226,8 +228,8 @@ class ReportUIFeatures { | |||
this.headerSticky.style.transform = `translateY(${heightDiff * scrollPct * -1}px)`; | |||
this.headerBackground.style.transform = `translateY(${scrollPct * this.headerOverlap}px)`; | |||
this.lighthouseIcon.style.transform = | |||
`translate3d(var(--report-width-half),` + | |||
` calc(-100% - ${scrollPct * this.headerOverlap * -1}px), 0) scale(${1 - scrollPct})`; | |||
`translate3d(0,` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might have to explain this one to me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the lh-lighthouse
element is inside the header container now it doesn't need to be translated on the x axis since it is positioned absolutely to the bottom right corner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the 100%
is gone because of the bottom right corner position too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it had -100% on in the .lh-lighthouse
class to position it before, so the new anchoring doesn't need it.
@@ -728,6 +731,7 @@ | |||
|
|||
.lh-report { | |||
background-color: #fff; | |||
min-width: var(--report-min-width); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this used to be only @media screen
do we still want to keep that or doesn't matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't think it really matters. I moved it here because I felt like it was always true for every screen not just max-width: 964px
but I'm not sure it matters?
top: var(--report-header-height); | ||
right: 50%; | ||
transform: translate3d(var(--report-width-half), -100%, 0); | ||
bottom: -4px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bottom -4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.lh-lighthouse { | ||
position: absolute; | ||
top: var(--report-header-height); | ||
right: 50%; | ||
transform: translate3d(var(--report-width-half), -100%, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like these all changed because lh-lighthouse
moved inside the header. what does that get us and do we know if there's a reason it was outside it to begin with? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % nit
@@ -164,11 +164,13 @@ | |||
width: 100%; | |||
will-change: transform; | |||
} | |||
.lh-narrow .lh-header-bg { | |||
height: var(--report-header-height-narrow); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two nits (that may or may not be correct :) but otherwise LGTM! 🔛
Transition from full width to >500px
minimum width without horizontal scroll (400px):
minimum width without horizontal scroll (1 category):
Related Issues/PRs
fixes #6645