Sidebar styling tweaks #703

dutchmartin · 2018-08-09T09:47:31Z

Summary

This PR can be summarized in the following changelog entry:

Changed the border color of the Collapsible component to #e2e4e7.
Changed the text color of the Collapsibles within the ContentAnalysis component to $color_headings (#555) and remove the bold font-weight.
Removed the max-width from the ContentAnalysis component to remove the wrapping of the analysis results on big screens
Added a dropshadow to the YoastInputField component.

Relevant technical choices:

Adding a color in the colors.json.

Test instructions

This PR can be tested by following these steps:

Checkout this branch and link it into wordpress seo.
Go to a post.
Verify the following things:
- The dividers between the sidebar sections have the same color as Gutenberg uses, i.e. #e2e4e7.
- The titles within the analysis have the same font color as the analysis descriptions and are not bold.
- All our input fields in the sidebar and metabox (keywords, synonyms, additional focus keyword, snippet editor title, snippet editor description) have the same blue border on focus.
- All keywords and synonyms inputs in the metabox and sidebar have the same height: 16 px.
- The analysis component doesn't have a max-width from, meaning the analysis results no longer wrap on bigger screens.

Fixes Yoast/wordpress-seo#10596

IreneStr

CR done: 1 minor comment. Also, please solve the merge conflicts.

IreneStr · 2018-08-14T08:23:42Z

composites/Plugin/Shared/components/Collapsible.js

@@ -65,6 +66,7 @@ const StyledTitle = styled.span`
 	white-space: nowrap;
 	text-overflow: ellipsis;
 	overflow-x: hidden;
+	color: ${ colors.$palette_grey_dark };


Please never use a $palette_... color. Use $color_... instead. In this case: $color_headings

Resolve the things that are moved upstream.

Dieterrr

Please fix the travis builds that fail.
I don't think it's necessary to add a color to the style guide for this issue but if you think it is important make sure design approves.

Dieterrr · 2018-08-17T09:37:30Z

style-guide/_colors.scss

@@ -2,6 +2,7 @@ $palette_white:              #fff;    // Safe to use with $palette_grey_text –
 // Shades of gray from lighter to darker.
 $palette_grey_ultra_light:   #f7f7f7; // Safe to use with $palette_grey_text – $palette_black.
 $palette_grey_light:         #f1f1f1; // Safe to use with $palette_grey_text – $palette_black.
+$palette_grey_medium_light:  #e2e4e7;


Did you discuss adding this color to the style guide with design? If not, please first make sure they are ok with it, otherwise use one of the existing colors from the style guide.

This has been discussed with @hedgefield and he was fine with it 😄 But good question to ask!

IreneStr · 2018-08-29T08:17:54Z

Acceptance 🚧

You've introduced palette_grey_medium_light but are not using it. The dividers are still too light grey.
You've introduced an !important in the CSS. Please ask for help to find a different solution. @afercia might be your best bet. Only when nothing else seems to work, use!important.
The focus keyword input field in the sidebar is styled completely differently than the other input fields: they have rounded corners and the border is too dark.

For the next tester: don't forget to test the multiple keyword input fields in premium.

Added comment at color `palette_grey_medium_light` to differentiate between colors from the Yoast Styleguide and the Gutenberg colors. This could be relevant when Gutenberg changes its colors, for example.

jcomack

CR done 👍

afercia · 2018-09-26T09:21:40Z

composites/Plugin/Shared/components/Collapsible.js

-	border-top: 1px solid ${ colors.$palette_grey_light };
-	border-bottom: 1px solid ${ colors.$palette_grey_light };
+	border-top: 1px solid ${ colors.$palette_grey_medium_light };
+	border-bottom: 1px solid ${ colors.$palette_grey_medium_light };


please do not use $palette_* variables directly; their aliases should be used instead. I see an alias $color_border_gutenberg has been created, it should be used here.

afercia · 2018-09-26T09:37:45Z

composites/Plugin/Shared/components/YoastInput.js

-	padding: 0.75em;
-	font-size: 1em;
+	&&& {
+		box-shadow: inset 0 1px 2px rgba(0,0,0,.07);


from the style-guide readme:

do not use rgba colors in the palette, instead use the rgba() function

I'd agree the wording of this recommendation could be improved. However, it's meant to not introduce in the codebase colors that are not part of the palette. In this case it's a minor issue because it's black but for consistency I'd recommend to use ${ rgba( colors.$color_black, 0.7 ) }

afercia · 2018-09-26T09:50:03Z

composites/Plugin/Shared/components/YoastInput.js

+	&&& {
+		box-shadow: inset 0 1px 2px rgba(0,0,0,.07);
+		border: 1px solid ${ colors.$color_input_border };
+		padding: 0.75em;


I know this value is pre-existing, but it was discussed at some point because em values don't make much sense in this context and don't match our 8 pixels based grid size. Both in the standalone app and in Gutenberg the computed value is 9.75px which will also be subject to browsers rounding. I'd consider to use a different value, in pixels /Cc @hedgefield

afercia · 2018-09-26T09:51:14Z

composites/Plugin/Shared/components/YoastInput.js

+		box-shadow: inset 0 1px 2px rgba(0,0,0,.07);
+		border: 1px solid ${ colors.$color_input_border };
+		padding: 0.75em;
+		font-size: 13px;


I'd keep it to 1em (which is equivalent to 13px in Gutenberg) unless there are specific reasons to use a value in pixels.

afercia · 2018-09-26T09:55:52Z

CR 🚧
See a few comments, mostly related to best practices.

afercia · 2018-09-28T11:57:39Z

Right now, this can't be tested on the plugin because:

the plugin trunk still has the getI18n error
the hotfix/8.3.1 branch that fixes the getI18n error has a wrong WPSEO_VERSION that prevents to build assets correctly
to actually test it, either the first or the second need to be manually fixed temporarily

afercia · 2018-10-01T08:21:56Z

composites/Plugin/Shared/components/YoastInput.js

@@ -18,7 +19,11 @@ export const YoastInputLabel = styled.label`
 `;

 export const YoastInputField = styled.input`
-	border: 1px solid ${ colors.$color_input_border };
 	padding: 0.75em;
 	font-size: 1em;


These padding and font-size will now be overridden by Gutenberg in the sidebar. If we want the input fields in the sidebar to have the same size of the ones in the metabox, these should be moved within the block with higher specificity. Also, the padding is still 0.75em, as mentioned in a previous comment that computes to 9.75 pixels based on the font size usd in that context. A value in pixels would be preferable.

Current size difference between the input field in the metabox and in the sidebar:

afercia · 2018-10-01T09:10:23Z

composites/Plugin/Shared/components/YoastInput.js

+	&&& {
+		box-shadow: inset 0 1px 2px ${ rgba( colors.$color_black, 0.07 ) };
+		border: 1px solid ${ colors.$color_input_border };
+		border-radius: 0;


Noticed that setting box-shadow and border to a so high specificity prevents the focus style to work properly in the metabox. Instead in the sidebar the focus style is inherited by Gutenberg and it works.

afercia · 2018-10-01T09:39:08Z

CR 👍

Pushed a couple changes to:

set the input field padding to 8 pixels as per design feedback
fix the input field focus style

Will split the hardcoded colors in a separate issue.

When doing acceptance, please verify the fields (keyphrase, additional keyphrase, synonyms) have the same styling in the metabox and in the sidebar, including the focus style:

IreneStr · 2018-10-01T11:12:42Z

Acceptance done 🎉

The dividers between the sidebar sections should have the same color as Gutenberg uses, i.e. a darker grey.
The titles within the analysis should have the same font color as the analysis descriptions (= black/ very dark grey) and shouldn't be bold.
All input fields in the sidebar (keywords, synonyms, additional focus keyword (more?)) should have the same style as the input fields in the snippet editor. This means that at least a dropshadow should be added).
Remove the max-width from the analysis component to remove the wrapping of the analysis results on big screens.

changed the collabsable border color to the one gutenberg uses

602eaa1

dutchmartin added the in progress label Aug 9, 2018

dutchmartin added 4 commits August 10, 2018 15:59

Added the right color to the colabsable heading

5e4af26

Add a box shadow to the input field.

7c3025a

Merge remote-tracking branch 'origin' into 10596-sidebar-polishing

6e5d8ec

Update the snapshots to fix te unit tests.

3bb3d7a

IreneStr reviewed Aug 14, 2018

View reviewed changes

dutchmartin added 6 commits August 14, 2018 11:32

Merge branch 'develop' into 10596-sidebar-polishing

422ef4d

Resolve the things that are moved upstream.

Build the pr with the new upstream changes

85ec545

Override the gutenberg css by using important

511348d

Fix the failing unit tests.

f10a5ac

Merge remote-tracking branch 'origin' into 10596-sidebar-polishing

b43a60a

Moved the color since someone else moved the section title.

c32f11e

Dieterrr suggested changes Aug 17, 2018

View reviewed changes

Update jest snapshots

d2155dd

IreneStr removed the in progress label Aug 29, 2018

IreneStr assigned dutchmartin and unassigned dutchmartin Aug 29, 2018

IreneStr mentioned this pull request Sep 3, 2018

Sidebar styling tweaks Yoast/wordpress-seo#10596

Closed

5 tasks

moorscode changed the title ~~Fix the sidebar polishing issues~~ Sidebar styling tweaks Sep 3, 2018

Dieterrr and others added 9 commits September 4, 2018 16:10

Added comment at new color.

774b5f0

Added comment at color `palette_grey_medium_light` to differentiate between colors from the Yoast Styleguide and the Gutenberg colors. This could be relevant when Gutenberg changes its colors, for example.

Merge branch 'develop' into 10596-sidebar-polishing

ba66047

Increased YoastInput CSS specificity

0447eee

Made YoastInput styling more aggressive to overwrite Gutenberg styling

8501cd3

Made dividers between collapsibles darker

dc38a5d

Fixed reference to yoastseo

c97091e

Updated snapshots

aada685

Merge branch 'develop' into 10596-sidebar-polishing

82f6ade

Fixed incorrect yarn.lock file

a2d4281

jcomack approved these changes Sep 25, 2018

View reviewed changes

afercia reviewed Sep 26, 2018

View reviewed changes

abotteram added 2 commits September 26, 2018 13:25

Applied CR feedback

84b8ac5

Updated scnapshot

f608b58

afercia reviewed Oct 1, 2018

View reviewed changes

Standardize input field padding to 8px as per design feedback.

ebf90fb

afercia reviewed Oct 1, 2018

View reviewed changes

Fix the focus style.

4aa4178

IreneStr merged commit a3f623c into develop Oct 1, 2018

IreneStr deleted the 10596-sidebar-polishing branch October 1, 2018 12:08

This was referenced Oct 1, 2018

Standardize input fields padding to 8 pixels #747

Open

Audit hardcoded colors and replace them with variables #748

Open

IreneStr added this to the 4.13 milestone Oct 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sidebar styling tweaks #703

Sidebar styling tweaks #703

dutchmartin commented Aug 9, 2018 •

edited by IreneStr

IreneStr left a comment

IreneStr Aug 14, 2018

Dieterrr left a comment

Dieterrr Aug 17, 2018

IreneStr Aug 27, 2018

IreneStr commented Aug 29, 2018

jcomack left a comment

afercia Sep 26, 2018

afercia Sep 26, 2018

afercia Sep 26, 2018

afercia Sep 26, 2018

afercia commented Sep 26, 2018

afercia commented Sep 28, 2018

afercia Oct 1, 2018

afercia Oct 1, 2018

afercia commented Oct 1, 2018

IreneStr commented Oct 1, 2018 •

edited

Sidebar styling tweaks #703

Sidebar styling tweaks #703

Conversation

dutchmartin commented Aug 9, 2018 • edited by IreneStr

Summary

Relevant technical choices:

Test instructions

IreneStr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dieterrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IreneStr commented Aug 29, 2018

jcomack 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

afercia commented Sep 26, 2018

afercia commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Oct 1, 2018

IreneStr commented Oct 1, 2018 • edited

dutchmartin commented Aug 9, 2018 •

edited by IreneStr

IreneStr commented Oct 1, 2018 •

edited