Fix Bug 1453479 - more dark theme updates by rlr · Pull Request #4087 · mozilla/activity-stream

rlr · 2018-04-12T12:32:08Z

No description provided.

rlr · 2018-04-12T12:33:58Z

I have a few outstanding questions to Amy still. But maybe getting more eyeballs on this will create more? @Mardak

rlr · 2018-04-12T12:36:07Z

  height: $search-height;
-  // The extra 1px is to account for the box-shadow being outside of the element
-  // instead of inside. It needs to be like that to not overlap the inner background
-  // color of the hover state of the submit button.


I tried to reproduce this code comment but I couldnt see a difference? So I removed the horizontal margin so the dropdown is the right width.

Hmm.. @k88hudson added that comment in #3785.. Doesn't look like either 1px need to be around anymore?

rlr · 2018-04-12T15:16:32Z

@Mardak ok, I think this is ready for 👁 👁 . Hopefully the last "major" round of changes for now 😝 #tedious

rlr · 2018-04-12T16:18:46Z

  --newtab-textbox-border: $grey-90-20;
-  --newtab-textbox-color: inherit;
+  --newtab-textbox-focus-color: $blue-60;
+  --newtab-textbox-focus-boxshadow: 0 0 0 1px $blue-60, 0 0 0 4px rgba($blue-60, 0.3); // sass-lint:disable-line no-color-literals


I don't like this but... is there a cleaner solution?

Are you referring to the duplicate stuff between default and dark? Maybe something like:

diff --git a/system-addon/content-src/styles/_theme.scss b/system-addon/content-src/styles/_theme.scss index eb04f699..97bccdcc 100644 --- a/system-addon/content-src/styles/_theme.scss +++ b/system-addon/content-src/styles/_theme.scss @@ -1 +1,6 @@ +@mixin textbox-focus($color) { + --newtab-textbox-focus-color: $color; + --newtab-textbox-focus-boxshadow: 0 0 0 1px $color, 0 0 0 4px rgba($color, 0.3); // sass-lint:disable-line no-color-literals +} + // Default theme @@ -21,4 +26,3 @@ body { --newtab-textbox-border: $grey-90-20; - --newtab-textbox-focus-color: $blue-60; - --newtab-textbox-focus-boxshadow: 0 0 0 1px $blue-60, 0 0 0 4px rgba($blue-60, 0.3); // sass-lint:disable-line no-color-literals + @include textbox-focus($blue-60); @@ -74,4 +78,3 @@ body { --newtab-textbox-border: $grey-10-20; - --newtab-textbox-focus-color: $blue-40; - --newtab-textbox-focus-boxshadow: 0 0 0 1px $blue-40, 0 0 0 4px rgba($blue-40, 0.3); // sass-lint:disable-line no-color-literals + @include textbox-focus($blue-40);

rlr · 2018-04-12T16:23:58Z

Focused search box now looks like this (+ Amy approved):

Mardak · 2018-04-13T09:11:31Z

  --newtab-textbox-border: $grey-90-20;
-  --newtab-textbox-color: inherit;
+  --newtab-textbox-focus-color: $blue-60;
+  --newtab-textbox-focus-boxshadow: 0 0 0 1px $blue-60, 0 0 0 4px rgba($blue-60, 0.3); // sass-lint:disable-line no-color-literals


Are you referring to the duplicate stuff between default and dark? Maybe something like:

diff --git a/system-addon/content-src/styles/_theme.scss b/system-addon/content-src/styles/_theme.scss index eb04f699..97bccdcc 100644 --- a/system-addon/content-src/styles/_theme.scss +++ b/system-addon/content-src/styles/_theme.scss @@ -1 +1,6 @@ +@mixin textbox-focus($color) { + --newtab-textbox-focus-color: $color; + --newtab-textbox-focus-boxshadow: 0 0 0 1px $color, 0 0 0 4px rgba($color, 0.3); // sass-lint:disable-line no-color-literals +} + // Default theme @@ -21,4 +26,3 @@ body { --newtab-textbox-border: $grey-90-20; - --newtab-textbox-focus-color: $blue-60; - --newtab-textbox-focus-boxshadow: 0 0 0 1px $blue-60, 0 0 0 4px rgba($blue-60, 0.3); // sass-lint:disable-line no-color-literals + @include textbox-focus($blue-60); @@ -74,4 +78,3 @@ body { --newtab-textbox-border: $grey-10-20; - --newtab-textbox-focus-color: $blue-40; - --newtab-textbox-focus-boxshadow: 0 0 0 1px $blue-40, 0 0 0 4px rgba($blue-40, 0.3); // sass-lint:disable-line no-color-literals + @include textbox-focus($blue-40);

Mardak · 2018-04-13T09:16:35Z


  &.active {
-    background: var(--newtab-element-active-color);
+    background: var(--newtab-element-hover-color);


Somewhat confusing that .active uses -hover-color.. oh well

we can rename the css class to 🤔 something.

Eh, we can just leave it :p

Mardak · 2018-04-13T09:23:05Z

  height: $search-height;
-  // The extra 1px is to account for the box-shadow being outside of the element
-  // instead of inside. It needs to be like that to not overlap the inner background
-  // color of the hover state of the submit button.


Hmm.. @k88hudson added that comment in #3785.. Doesn't look like either 1px need to be around anymore?

Mardak · 2018-04-13T09:25:10Z

 $input-error-border: solid 1px $red-60;
 $input-error-boxshadow: 0 0 0 1px $red-60, 0 0 0 4px rgba($red-60, 0.3);
-$input-focus-boxshadow: 0 0 0 1px $blue-50, 0 0 0 4px rgba($blue-50, 0.3);
+$input-focus-boxshadow: var(--newtab-textbox-focus-boxshadow);


If it's just the var, probably just directly use var(--newtab-textbox-focus-boxshadow) and var(--newtab-card-shadow) without the sass variable. Functions wouldn't really be able to do stuff with the value anyway.

Mardak · 2018-04-13T09:30:54Z

 $os-infopanel-arrow-height: 10px;
 $os-infopanel-arrow-offset-end: 6px;
 $os-infopanel-arrow-width: 20px;
-$os-search-focus-shadow-radius: 1px;


Is this removal actually desired? It regresses #3745 which said to have 1px for windows.

I asked Amy explicitly about it. I'll double confirm.

the UX powers say we can get rid of the windows specific special case 🎉

Mardak · 2018-04-13T09:32:07Z

  --newtab-button-primary-color: $blue-60;
  --newtab-button-secondary-color: inherit;
  --newtab-element-active-color: $grey-20;
+  --newtab-element-hover-color: $grey-20;


There should be no differentiation from hover and clicking on a context menu item for default theme?

Oh right, it needs to get darker. 👍

Mardak

Just a bit more cleanup! Thanks

Mardak · 2018-04-13T19:00:02Z

 $shadow-primary: 0 0 0 5px var(--newtab-card-active-outline-color);
-$shadow-secondary: 0 1px 4px 0 $grey-90-10;
+$shadow-secondary: 0 1px 4px 0 $grey-90-20;
+$shadow-card: var(--newtab-card-shadow);


This sass variable can go too

Mardak · 2018-04-13T19:00:43Z

-  // instead of inside. It needs to be like that to not overlap the inner background
-  // color of the hover state of the submit button.
-  margin: 1px 1px $section-spacing;
+  margin: 1px 0 $section-spacing;


k88 says if it looks right, then good to go, and from my testing, it seems fine with margin-bottom: $section-spacing;

rlr · 2018-04-13T19:49:30Z

^ I also added :active state for the prefs gear icon

Mardak · 2018-04-13T21:07:38Z

+$input-border-active: solid 1px var(--newtab-textbox-focus-color);
 $input-error-border: solid 1px $red-60;
 $input-error-boxshadow: 0 0 0 1px $red-60, 0 0 0 4px rgba($red-60, 0.3);
-$input-focus-boxshadow: 0 0 0 1px $blue-50, 0 0 0 4px rgba($blue-50, 0.3);


Oh, I forgot to note that I think sass allows a custom function where we can pass in a $red-60 or $blue-40/60/$color to generate the 0 0 0 1px $color, 0 0 0 4px rgba($color, 0.3) value.. maybe.......... ?

It works! And even avoids the disable-line ;)

diff --git a/system-addon/content-src/styles/_theme.scss b/system-addon/content-src/styles/_theme.scss --- a/system-addon/content-src/styles/_theme.scss +++ b/system-addon/content-src/styles/_theme.scss @@ -1,4 +1,8 @@ +@function textbox-shadow($color) { + @return 0 0 0 1px $color, 0 0 0 4px rgba($color, 0.3); +} + @mixin textbox-focus($color) { --newtab-textbox-focus-color: $color; - --newtab-textbox-focus-boxshadow: 0 0 0 1px $color, 0 0 0 4px rgba($color, 0.3); // sass-lint:disable-line no-color-literals + --newtab-textbox-focus-boxshadow: textbox-shadow($color); } @@ -118,3 +122,3 @@ $input-border-active: solid 1px var(--newtab-textbox-focus-color); $input-error-border: solid 1px $red-60; -$input-error-boxshadow: 0 0 0 1px $red-60, 0 0 0 4px rgba($red-60, 0.3); +$input-error-boxshadow: textbox-shadow($red-60); $inner-box-shadow: 0 0 0 1px var(--newtab-inner-box-shadow-color);

as-pine-proxy · 2018-04-15T16:21:53Z

Treeherder: 57cd3ef0812be24a627067bff4bbb8baed5efcf2

rlr commented Apr 12, 2018

View reviewed changes

rlr force-pushed the Bug1453479/dark-theme-updates branch from 7ca9af4 to e60dd22 Compare April 12, 2018 14:57

rlr requested a review from Mardak April 12, 2018 15:15

rlr assigned Mardak Apr 12, 2018

rlr added the PR / Needs review label Apr 12, 2018

rlr force-pushed the Bug1453479/dark-theme-updates branch from 8a593ac to 2ae1230 Compare April 12, 2018 16:17

rlr commented Apr 12, 2018

View reviewed changes

Mardak reviewed Apr 13, 2018

View reviewed changes

Mardak approved these changes Apr 13, 2018

View reviewed changes

Mardak assigned rlr and unassigned Mardak Apr 13, 2018

Mardak added PR / R+ with nits and removed PR / Needs review labels Apr 13, 2018

Fix Bug 1453479 - more dark theme updates

180d534

rlr force-pushed the Bug1453479/dark-theme-updates branch from e03ee52 to 180d534 Compare April 13, 2018 20:05

rlr merged commit 030284a into mozilla:master Apr 13, 2018

Mardak reviewed Apr 13, 2018

View reviewed changes

Mardak mentioned this pull request Apr 13, 2018

chore(styles): Refactor textbox shadow from color rule #4091

Merged

rlr deleted the Bug1453479/dark-theme-updates branch April 13, 2018 22:13

as-pine-proxy added the pushed-to-pine label Apr 15, 2018

Conversation

rlr commented Apr 12, 2018

Uh oh!

rlr commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlr commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlr commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mardak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlr commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

as-pine-proxy commented Apr 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants