report: create naming convention for css variables #9149

connorjclark · 2019-06-07T18:32:48Z

The only visual change should be this layout fix (see that bad chevron):

Before

After

Open to discussion. Note, I will follow up with sorting. Some stuff to start us off:

First off, thoughts on the naming convention?
For variables that affect the entire report, what "component" name should be used? We kinda used body before, but I don't like that because places like PSI / DevTools don't provide the entire body to the report. I'm thinking either report or default.
Should everything be prepended with lh-?
Should we be using only Material Design colors?
Most of these variables were simple to rename. I had trouble with some, which I marked.

patrickhulce

love that we're standardizing these thanks @hoten! 🎉 💯

patrickhulce · 2019-06-10T17:12:20Z

lighthouse-core/report/html/report-styles.css

@@ -15,41 +15,56 @@
 * limitations under the License.
 */

+/*
+  Naming convention:


patrickhulce · 2019-06-10T17:14:02Z

lighthouse-core/report/html/report-styles.css

+  across multiple components, either create more variables or just drop the "{component}-"
+  part of the name. Append any modifiers at the end (ex: 'big', 'dark').
+
+  For colors: --color-{type}-{hue}


seems like you mean intensity/lightness/saturation here instead of hue, type is currently the hue :)

Yes, thanks!

I couldn't find an MD lingo for describing those tags (700, A700, ...)

patrickhulce · 2019-06-10T17:15:45Z

lighthouse-core/report/html/report-styles.css

-  --section-indent: 16px;
-  --audit-item-gap: 5px;
-  --text-indent: 8px;
+  --footer-vertical-padding: 16px;


seems like we're a tad loose with property-name should this be --footer-padding-vertical according to the convention? maybe add this as a counter example to the comment if not?

Yeah that makes sense. padding-vertical isn't a property, but padding-{top,bottom} is, and padding-vertical follows from using the "common descriptor" rule.

patrickhulce · 2019-06-10T17:17:17Z

lighthouse-core/report/html/report-styles.css

-  --lh-audit-vpadding: 4px;
-  --lh-audit-group-vpadding: 12px;
-  --lh-section-vpadding: 8px;
+  --lh-audit-vertical-padding: 4px;


same deal here with padding-vertical vs. vertical-padding

patrickhulce · 2019-06-10T17:18:39Z

lighthouse-core/report/html/report-styles.css

@@ -320,7 +335,7 @@
 }

 .lh-audit__description, .lh-audit__stackpack {
-  --inner-audit-right-padding: calc(var(--text-indent) + 2px);
+  --inner-audit-right-padding: var(--stackpack-horizontal-padding);


elsewhere we used margin-right which seemed to align with the convention, flip here too to match property name?

patrickhulce

LGTM as first pass on our new naming convention!

TODOs are followups?

patrickhulce · 2019-06-10T18:31:29Z

lighthouse-core/report/html/report-styles.css

+  across multiple components, either create more variables or just drop the "{component}-"
+  part of the name. Append any modifiers at the end (ex: 'big', 'dark').
+
+  For colors: --color-{type}-{intensity}


would probably go with {hue} over {type} since I'm not sure what type means in the context of a color, I know my last comment was kinda ambiguous on that sorry!

patrickhulce · 2019-06-10T18:32:31Z

Oh and this looks pre-existing but the large amount of padding on our failure audits jumped out as unusual to me when doing a quick visual check

connorjclark · 2019-06-10T18:38:25Z

Oh and this looks pre-existing but the large amount of padding on our failure audits jumped out as unusual to me when doing a quick visual check

Confirmed it exists in a different PR's now deployment, so yeah pre-existing.

Empty display text wants some space too.

connorjclark · 2019-06-10T18:42:04Z

TODOs are followups?

Would like to resolve some of them here. The ones that just say TODO: rename I couldn't figure out a good name for them, any ideas?

also, if we don't want to prefix all vars with lh- I'll remove the ones that do have it.

patrickhulce · 2019-06-10T18:53:02Z

lighthouse-core/report/html/report-styles.css

-  --audit-item-gap: 5px;
-  --text-indent: 8px;
+  --footer-padding-vertical: 16px;
+  --audit-item-gap: 5px; /* TODO rename. */


audit-margin-horizontal?

patrickhulce · 2019-06-10T18:56:53Z

lighthouse-core/report/html/report-styles.css

  --secondary-text-color: var(--color-black-800);
  --informative-color: var(--color-blue-900);
-  --medium-75-gray: #757575;
-  --medium-50-gray: hsl(210, 17%, 98%);
+  --medium-75-gray: #757575; /* TODO rename / switch to MD */


I think this is exactly --color-gray-600

right you are

patrickhulce · 2019-06-10T18:57:59Z

lighthouse-core/report/html/report-styles.css

-  --medium-75-gray: #757575;
-  --medium-50-gray: hsl(210, 17%, 98%);
+  --medium-75-gray: #757575; /* TODO rename / switch to MD */
+  --medium-50-gray: hsl(210, 17%, 98%); /* TODO rename / switch to MD */


--color-gray-50: #fafafa instead?

patrickhulce · 2019-06-10T18:59:00Z

lighthouse-core/report/html/report-styles.css

  --chevron-size: 12px;
-  --inner-audit-left-padding: calc(var(--score-shape-size) + var(--score-shape-margin-left) + var(--score-shape-margin-right));
+  --inner-audit-left-padding: calc(var(--score-icon-size) + var(--score-icon-margin-left) + var(--score-icon-margin-right)); /* TODO: rename */


--audit-description-padding-left?

paulirish

i really like having this convention defined. thanks for working through it!

paulirish · 2019-06-11T23:39:08Z

lighthouse-core/report/html/report-styles.css

+
+  {intensity} is the Material Design tag - 700, A700, etc. Omit "-{intensity}" if there is only one
+  color of that hue.
+*/
 .lh-vars {
  --text-font-family: Roboto, Helvetica, Arial, sans-serif;
  --monospace-font-family: 'Roboto Mono', 'Menlo', 'dejavu sans mono', 'Consolas', 'Lucida Console', monospace;
  --body-background-color: #fff;
  --body-text-color: var(--color-black-900);


according to the convention, should all --body- be --report- now?

Let's do --report-.

paulirish · 2019-06-11T23:44:04Z

lighthouse-core/report/html/report-styles.css

 .lh-vars {
  --text-font-family: Roboto, Helvetica, Arial, sans-serif;
  --monospace-font-family: 'Roboto Mono', 'Menlo', 'dejavu sans mono', 'Consolas', 'Lucida Console', monospace;
  --body-background-color: #fff;
  --body-text-color: var(--color-black-900);
  --body-font-size: 16px;
  --body-line-height: 24px;
-  --subheader-color: hsl(206, 6%, 25%);
-  --header-font-size: 20px;
+  --snippet-color: hsl(206, 6%, 25%);


this probably should just adopt an existing color. seems odd to have it be off on its own

paulirish · 2019-06-11T23:44:29Z

lighthouse-core/report/html/report-styles.css

-  --text-indent: 8px;
+  --footer-padding-vertical: 16px;
+  --audit-margin-horizontal: 5px;
+  --stackpack-padding-horizontal: 10px;
  --secondary-text-color: var(--color-black-800);


--color-... ? or does that only apply for the "root" colors?

I could go either way with this. The two ways to interpret...

--secondary-text-color, if you consider secondary-text to be a "component"...

--color-secondary-text, because anytime you reach out for a color it's nice to just do --color and look at autocomplete. The average / fail / etc. colors already are like this.

I'm leaning towards 2), plus expanding the color naming rule to cover these.

paulirish · 2019-06-11T23:44:55Z

lighthouse-core/report/html/report-styles.css

-  --medium-50-gray: hsl(210, 17%, 98%);
+  --color-gray-600: #757575;
+  --color-gray-50: #FAFAFA;
+  --snippet-background-color: var(--color-gray-50);


lets just use --color-gray-50 directly?

There's actually a few of the color component variables defined in this manner already:

--report-text-color: var(--color-black-900); --informative-color: var(--color-blue-900); --snippet-background-color: var(--color-gray-50); --tools-icon-color: var(--color-gray-600); --env-item-background-color: var(--color-black-100); --plugin-badge-background-color: var(--color-white); --topbar-background-color: var(--color-black-100); --metrics-toggle-background-color: var(--color-black-200);

which is kinda nice because inverting the --color- variables is all that needs to be done to do dark theming.

or do you mean removing the variable --color-gray-50 and inlining the value here

paulirish · 2019-06-12T00:13:06Z

lighthouse-core/report/html/report-styles.css

  --report-secondary-border-color: #ebebeb;
-  --display-value-gray: hsl(216, 5%, 39%);
+  --chevron-line-stroke: hsl(216, 5%, 39%);
  --report-width: calc(60 * var(--body-font-size));
  --report-min-width: 400px;
  --report-header-height: 161px;


this is the only --report- prefixed item that refers to a specific component..

oh look at that.. it's actually outdated and old now. PR drafted: #9188

paulirish · 2019-06-12T00:17:30Z

lighthouse-viewer/app/styles/viewer.css

@@ -9,7 +9,7 @@
 }

 :root {
-  --lh-background-color: #304ffe;


seems like this isn't used anymore. i think it can die.

I'm actually gonna revert it b/c I don't want to touch the viewer at all rn.

connorjclark · 2019-06-12T00:46:34Z

lighthouse-core/report/html/report-styles.css

@@ -39,7 +39,7 @@
  --report-text-color: var(--color-black-900);
  --report-font-size: 16px;
  --report-line-height: 24px;
-  --snippet-color: hsl(206, 6%, 25%);
+  --snippet-color: var(--color-black-800);


look at Background and foreground colors do not have a sufficient contrast ratio. in the now deployment for a snippet.

connorjclark · 2019-06-12T01:00:19Z

ok, another visual change made its way in. we were not changing the cheveron color in dark theme

before

after

brendankenny

looks good, it's nice to have a system.

What to pick as the component seems like it will definitely be ambiguous at times. And without scoping, variables like --plugin-icon-size are still just kind of out there (ideally you'd use css to scope them, but that makes overriding in different breakpoints/environments messier).

It's also somewhat BEM like, so I wonder if we should adopt the underscore/hyphen/double-hyphen conventions from there, but I also don't care either way about BEM so I'm fine with whatever :)

brendankenny · 2019-06-14T19:41:35Z

lighthouse-core/report/html/report-styles.css

-  --color-black-900: #212121;
-  --color-black: #000000;
-  --color-blue: #2962FF;
+  --color-gray-50: #FAFAFA;


I wonder how we started using gray instead of grey :)

fwiw the MD sites all say gray. the correct way, coincidentally

fwiw the MD sites all say gray.

ha, well the third party site in the comment above doesn't :)

the correct way, coincidentally

freedom is the only way, yeah

brendankenny · 2019-06-14T19:42:37Z

lighthouse-core/report/html/report-styles.css

  --color-green-700: #018642;
-  --color-green: #0CCE6B;
+  --color-green: #0CCE6B; /* TODO: this is not a MD color */


/* TODO: this is not a MD color */

do we care?

goood question. calling @yuinchien - should we only use colors from Material design?

brendankenny · 2019-06-14T19:44:57Z

lighthouse-core/report/html/report-styles.css

+  For colors: --color-{hue}-{intensity}
+
+  {intensity} is the Material Design tag - 700, A700, etc. Omit "-{intensity}" if there is only one
+  color of that hue.


if this gets longer or really applies to multiple places it could go in CONTRIBUTING.md, but seems ok here now

brendankenny · 2019-06-14T19:47:30Z

lighthouse-core/report/html/report-styles.css

-  --body-line-height: 24px;
-  --subheader-color: hsl(206, 6%, 25%);
-  --header-font-size: 20px;
+  --report-background-color: #fff;


since these are all changing in blame anyways, does it make sense to sort these, at least grouping by component?

May want to do that at the very end, though, cause the diff will be hard to follow

I want to wait until a completely separate PR, so it's in two commits and separates concerns in the PRs. Want to be able to follow through any changes in UIs like github's.

brendankenny · 2019-06-14T19:48:17Z

lighthouse-core/report/html/report-styles.css

+  --env-item-padding: 10px 0px;
+  --env-item-font-size: 28px;
+  --env-item-line-height: 36px;
+  --metric-description-padding: 0 0 2px calc(var(--score-icon-margin-left) + var(--score-icon-size) + var(--score-icon-margin-right));
  --gauge-circle-size-big: 112px;


should these be --score-gauge-* like the other gauge vars? Or the others should be --gauge-*?

brendankenny · 2019-06-14T19:50:20Z

lighthouse-core/report/html/report-styles.css

+  --score-gauge-percentage-font-size: 28px;
+  --score-icon-margin-left: 4px;
+  --score-icon-margin-right: 12px;
+  --score-icon-margin: 0 var(--score-icon-margin-right) 0 var(--score-icon-margin-left);


can we pick different prefix than score for one or both of the score things since they're quite different?

changed --score-gauge to --gauge

brendankenny · 2019-06-14T19:51:18Z

lighthouse-core/report/html/report-styles.css

-    --body-line-height: 20px;
+    --audit-group-margin-bottom: 20px;
+    --report-font-size: 14px;
+    --report-line-height: 20px;
    --env-name-min-width: 120px;
    --gauge-circle-size-big: 96px;
    --gauge-circle-size: 72px;
    --header-padding: 16px 0 16px 0;
    --plugin-icon-size: 75%;


kebab case does present kind of a problem for parsing component vs property, but that's not new here

report: create naming convention for css variables

102b064

connorjclark requested a review from paulirish as a code owner June 7, 2019 18:32

googlebot added the cla: yes label Jun 7, 2019

Merge remote-tracking branch 'origin/master' into styles-refactor-part-2

6838ae8

vercel bot deployed to staging June 10, 2019 17:03 View deployment

patrickhulce reviewed Jun 10, 2019

View reviewed changes

pr changes

05d0248

vercel bot deployed to staging June 10, 2019 17:30 View deployment

patrickhulce approved these changes Jun 10, 2019

View reviewed changes

hue

5149271

vercel bot deployed to staging June 10, 2019 18:34 View deployment

patrickhulce reviewed Jun 10, 2019

View reviewed changes

pr

611cc4b

vercel bot deployed to staging June 10, 2019 20:41 View deployment

rename

e2b380e

vercel bot deployed to staging June 10, 2019 20:42 View deployment

a700

93e4d59

vercel bot deployed to staging June 10, 2019 20:44 View deployment

remove --lh-

26854cc

connorjclark requested a review from brendankenny as a code owner June 10, 2019 20:50

vercel bot deployed to staging June 10, 2019 20:50 View deployment

revert extension changes

87dc304

vercel bot deployed to staging June 10, 2019 21:09 View deployment

Merge remote-tracking branch 'origin/master' into styles-refactor-part-2

d1b45f5

vercel bot deployed to staging June 11, 2019 22:36 View deployment

paulirish reviewed Jun 12, 2019

View reviewed changes

--report

79a43ff

vercel bot deployed to staging June 12, 2019 00:25 View deployment

revert viewer changes

d7a3b50

vercel bot deployed to staging June 12, 2019 00:39 View deployment

vercel bot deployed to staging June 12, 2019 00:45 View deployment

connorjclark commented Jun 12, 2019

View reviewed changes

there is no MD black

8e6209a

connorjclark force-pushed the styles-refactor-part-2 branch from a57fef5 to 8e6209a Compare June 12, 2019 00:54

vercel bot deployed to staging June 12, 2019 00:54 View deployment

chevron dark mode

ac71f78

vercel bot deployed to staging June 12, 2019 00:56 View deployment

fix color gray dark inversion

72eace2

vercel bot deployed to staging June 12, 2019 00:58 View deployment

Merge remote-tracking branch 'origin/master' into styles-refactor-part-2

08fac45

vercel bot deployed to staging June 12, 2019 02:00 View deployment

brendankenny reviewed Jun 14, 2019

View reviewed changes

pr

f4469bc

vercel bot deployed to staging June 14, 2019 20:07 View deployment

remove comment

9669e2a

vercel bot deployed to staging June 18, 2019 04:00 View deployment

connorjclark merged commit f8d30cf into master Jun 18, 2019

connorjclark added a commit that referenced this pull request Jun 19, 2019

report: create naming convention for css variables (#9149)

47213a7

paulirish mentioned this pull request Jun 19, 2019

report: logically order css variables #9232

Merged

connorjclark added a commit that referenced this pull request Jun 19, 2019

report: naming convention (#9149) followup fixes

387dcd0

paulirish pushed a commit that referenced this pull request Jun 20, 2019

report: naming convention (#9149) followup fixes

37f7261

connorjclark deleted the styles-refactor-part-2 branch July 11, 2019 23:10

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

report: create naming convention for css variables #9149

report: create naming convention for css variables #9149

Conversation

connorjclark commented Jun 7, 2019 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jun 10, 2019

connorjclark commented Jun 10, 2019

connorjclark commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Jun 12, 2019

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Jun 7, 2019 •

edited

Loading