-
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(redesign): tooltip for category description #8840
Changes from 20 commits
d8616e6
4f49f3d
6ab11e8
481ad34
a51d517
deffec8
1238796
b769c58
7c7f0f5
90a6583
358b251
4931f5e
6c58907
1161fe8
89edcb6
09bda8c
b4d50a7
41db4bb
f9dfbab
670df6e
f4cedf7
02abf1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,8 +61,9 @@ | |
--navitem-hpadding: var(--body-font-size); | ||
--navitem-vpadding: calc(var(--navitem-line-height) / 2); | ||
--lh-score-highlight-bg: hsla(0, 0%, 90%, 0.2); | ||
--lh-score-icon-background-size: 24px; | ||
--lh-group-icon-background-size: var(--lh-score-icon-background-size); | ||
--lh-info-icon-background-size: 20px; | ||
--lh-score-icon-background-size: 24px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just -lh-score-icon-size ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw ur commenting on something i just reordered i added could rename stuff after all this tho |
||
--lh-export-icon-size: var(--lh-score-icon-background-size); | ||
--lh-export-icon-color: var(--medium-75-gray); | ||
--lh-score-margin: 12px; | ||
|
@@ -173,6 +174,8 @@ | |
--pwa-fast-reliable-color-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg"><g fill-rule="nonzero" fill="none"><circle fill="%230CCE6B" cx="12" cy="12" r="12"/><path d="M12 4.3l6.3 2.8v4.2c0 3.88-2.69 7.52-6.3 8.4-3.61-.88-6.3-4.51-6.3-8.4V7.1L12 4.3zm-.56 12.88l3.3-5.79.04-.08c.05-.1.01-.29-.26-.29h-1.96l.56-3.92h-.56L9.3 12.82c0 .03.07-.12-.03.07-.11.2-.12.37.2.37h1.97l-.56 3.92h.56z" fill="%23FFF"/></g></svg>'); | ||
--pwa-installable-color-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg"><g fill-rule="nonzero" fill="none"><circle fill="%230CCE6B" cx="12" cy="12" r="12"/><path d="M12 5a7 7 0 1 0 0 14 7 7 0 0 0 0-14zm3.5 7.7h-2.8v2.8h-1.4v-2.8H8.5v-1.4h2.8V8.5h1.4v2.8h2.8v1.4z" fill="%23FFF"/></g></svg>'); | ||
--pwa-optimized-color-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><rect fill="%230CCE6B" width="24" height="24" rx="12"/><path d="M5 5h14v14H5z"/><path fill="%23FFF" d="M12 15.07l3.6 2.18-.95-4.1 3.18-2.76-4.2-.36L12 6.17l-1.64 3.86-4.2.36 3.2 2.76-.96 4.1z"/></g></svg>'); | ||
|
||
--informative-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path fill="none" d="M0 0h24v24H0V0z"/><path fill="%23757575" d="M11 7h2v2h-2zm0 4h2v6h-2zm1-9C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm0 18c-4.41 0-8-3.59-8-8s3.59-8 8-8 8 3.59 8 8-3.59 8-8 8z"/></svg>'); | ||
} | ||
|
||
.lh-vars.dark { | ||
|
@@ -1139,6 +1142,16 @@ | |
will-change: opacity; /* Only using for layer promotion */ | ||
} | ||
|
||
/* Category header gauges shouldn't be links. */ | ||
.lh-category-wrapper .lh-gauge__wrapper { | ||
pointer-events: none; | ||
cursor: default; | ||
} | ||
.lh-gauge__label-info-icon { | ||
pointer-events: fill; | ||
cursor: help; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feels a tad odd to me to flip the cursor since there's no actual click action here, is there a broader UX strategy here? I would love to learn more about these kinds of things :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. honestly, I had the bring the cursor back with CSS so I just picked one. Since there's no click action to prompt perhaps it should just remain a normal cursor. IDK. @yuinchien what do you think of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hoten I suggest shifting the position 4px downward and to the right. I applied |
||
} | ||
|
||
.lh-gauge__label { | ||
font-size: var(--score-title-font-size); | ||
line-height: var(--score-title-line-height); | ||
|
@@ -1151,9 +1164,31 @@ | |
.lh-category .lh-gauge__label { | ||
--score-title-font-size: var(--score-title-font-size-big); | ||
--score-title-line-height: var(--score-title-line-height-big); | ||
display: flex; | ||
margin-top: 14px; | ||
} | ||
|
||
.lh-gauge__label-wrapper { | ||
position: relative; | ||
color: var(--body-text-color); | ||
} | ||
.lh-gauge__label-wrapper .tooltip-boundary { | ||
display: flex; | ||
position: absolute; | ||
right: calc(-1 * var(--lh-info-icon-background-size)); | ||
top: 14px; | ||
} | ||
.lh-gauge__label-wrapper .lh-gauge__label-info-icon { | ||
background-image: var(--informative-url); | ||
background-size: contain; | ||
width: var(--lh-info-icon-background-size); | ||
height: var(--lh-info-icon-background-size); | ||
} | ||
.lh-gauge__label-wrapper .tooltip { | ||
font-size: var(--caption-font-size); | ||
line-height: var(--caption-line-height); | ||
} | ||
|
||
|
||
.lh-scores-header .lh-gauge__wrapper, | ||
.lh-scores-header .lh-gauge--pwa__wrapper, | ||
|
@@ -1246,7 +1281,6 @@ | |
} | ||
|
||
.lh-category-header .lh-score__gauge { | ||
max-width: 400px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just unnecessary or do we want the gauges to get bigger than this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary |
||
width: auto; | ||
margin: 0px auto; | ||
} | ||
|
@@ -1445,7 +1479,7 @@ | |
position: absolute; | ||
display: none; /* Don't retain these layers when not needed */ | ||
opacity: 0; | ||
background: #ffffff; | ||
background: var(--body-background-color); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch |
||
min-width: 246px; | ||
max-width: 275px; | ||
padding: 15px; | ||
|
@@ -1470,7 +1504,6 @@ | |
display: block; | ||
animation: fadeInTooltip 250ms; | ||
animation-fill-mode: forwards; | ||
animation-delay: 850ms; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this removes this everywhere, we want that right? I would guess so because a lot of folks don't seem to know that they can hover over the error to get the tooltip 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I figured it doesn't make sense to inject 850ms of latency for this. |
||
bottom: 100%; | ||
z-index: 1; | ||
will-change: opacity; | ||
|
@@ -1481,7 +1514,7 @@ | |
.tooltip::before { | ||
content: ""; | ||
border: solid transparent; | ||
border-bottom-color: #fff; | ||
border-bottom-color: var(--body-background-color); | ||
border-width: 10px; | ||
position: absolute; | ||
bottom: -20px; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,7 +184,7 @@ describe('CategoryRenderer', () => { | |
const categoryDOM = renderer.render(category, sampleResults.categoryGroups); | ||
assert.ok(categoryDOM.querySelector('.lh-gauge__wrapper--plugin')); | ||
const label = categoryDOM.querySelector('.lh-gauge__label').textContent; | ||
assert.equal(category.title, label); | ||
assert.equal(label, category.title); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was backwards There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
just for code correctness purposes, not a functional change in the test, I hope? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
}); | ||
|
||
it('handles markdown in category descriptions a category', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,7 +242,7 @@ describe('PwaCategoryRenderer', () => { | |
describe('#renderScoreGauge', () => { | ||
it('renders an error score gauge in case of category error', () => { | ||
category.score = null; | ||
const badgeGauge = pwaRenderer.renderScoreGauge(category); | ||
const badgeGauge = pwaRenderer.renderScoreGauge(category, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the point of this one? typedef suggests both are unexpected but looking through the code it looks like we never used groups for the score gauge no matter what.... 🤷♂ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't recall, removed. |
||
|
||
// Not a PWA gauge. | ||
assert.strictEqual(badgeGauge.querySelector('.lh-gauge--pwa__wrapper'), null); | ||
|
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.
what's the point of moving this? (this same code is also in
pwa-category-renderer
)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.
It was suppressing the click event for links in the description. Only the header should be a clickable link, so the href= was moved to where the header gauges are made.
Should have removed the PWA one 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.
hows that
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.
that didn't work (just linked to top of page instead). Added a new 'disable' class as a quick fix. but maybe a better (but bigger) change could be done?