653 654 collapsible header icon keyword #676

afercia · 2018-07-30T07:17:50Z

Summary

This PR can be summarized in the following changelog entry:

Adds a subtitle to the collapsible header component

Relevant technical choices:

there was the need to make changes to the Collapsible component, consequently the ContentAnalysis needed some changes too
the new sub title is passed via the subTitle prop
removes AnalysisCollapsible (this should go in the changelog) and use Collapsible directly: AnalysisCollapsible was duplicating 95% of the code from Collapsible
adds new props to control the SVG icons, including their ViewBox: this is necessary because some icons have been coded with a different ViewBox size, thus affecting the way they're rendered
makes it possible to control the left and right icons independently
refactors ContentAnalysis and uses a renderCollapsible() helper function to simplify code
various CSS adjustments

Ideally, the Collapsible header component should be as style-agnostic as possible. For the header itself, it's difficult to style it independently other than passing props. Instead, the content of the collapsible should be styled independently using a specific styled component.

Partially fixes also #671 which will need some adjustments in the plugin. As requested, the left and right paddings should be set on the component itself. This means the StyledSection used in the plugin should be adjusted consequently. Screenshot of the analysis in the plugin (notice the left and right paddings to reset):

Test instructions

check the standalone app (yarn start and go to localhost:3333) and then go in the "Collapsibles" tab
verify all the collapsible examples there work as expected
note: in this page I've added a few styles to emulate the WordPress font metrics
note about the focus style: difficult because there's the need to override some styles that come from the button; I've made it in a way that there's also a subtile feedback when clicking the collapsible
note about the arrow icons on the right: difficult to align them vertically when there's a subtitle, ideally they should be shifted a few pixels to the bottom
go in the Content Analysis tab
verify everything works correctly
the Content Analysis collapsibles already implement some of the CSS adjustments requested in Styling improvements for the Analysis results. #671

Fixes #653
Fixes #654

abotteram

I mainly have some quarrels with a few decisions that have been made, that increase code complexity. Please address these concerns.

abotteram · 2018-07-30T09:13:59Z

composites/Plugin/Shared/components/Button.js

-	const { children: text, prefixIcon, prefixIconColor, suffixIcon, suffixIconColor } = props;
-
-	const newProps = omit( props, [ "prefixIcon", "prefixIconColor", "suffixIcon", "suffixIconColor" ] );
+	const {


I don't think we want to pass all these props to the button?
I suggest using object destructuring.

const { extractedProp1, extractedProp2, ...buttonProps } = props;

Also, what a godawful amount of props. Consider grouping prefix and suffix props, so you get props.suffix.icon, props.suffix.iconColor, and so on. I think this improved readability overall.

abotteram · 2018-07-30T09:15:41Z

composites/Plugin/Shared/components/Button.js

-
-	const newProps = omit( props, [ "prefixIcon", "prefixIconColor", "suffixIcon", "suffixIconColor" ] );
+	const {
+		children: text,


You rename the children prop explicitely to text, but the propTypes of the component suggest it can also be a element or multiple elements.

No I haven't touched this part, it was already children: text

abotteram · 2018-07-30T09:28:36Z

style-guide/_colors.scss

@@ -22,13 +22,14 @@ $palette_blue_dark:          #084a67; // Safe to use with $palette_white – $pa
 $palette_green:              #77b227; // Safe to use with $palette_black.
 $palette_green_light:        #7ad03a; // Safe to use with $palette_black.
 $palette_green_medium_light: #64a60a; // Safe to use with $palette_black or $palette_white bold > 18.5px.
-$palette_green_medium:       #008a00; // Safe to use with $palette_white and $palette_black.
+$palette_green_medium:       #008a00; // Safe to use with $palette_white and $palette_black. Also used for the smiley score icon.


Shouldn't we update the color_ok instead?

color_good, color_ok: yes we can update them but worth noting they're used also in other places, for example in the content analysis and for the progress bar. If we want to update there too, that's fine I guess.

abotteram · 2018-07-30T09:31:11Z

composites/Plugin/Shared/components/Collapsible.js

 const StyledIconsButton = styled( IconsButton )`
 	width: 100%;
 	background-color: ${ colors.$color_white };
-	padding: 15px;
+	padding: ${ props => props.headingPadding };


I think we should refrain from creating specific props for styling, can't we use a styled-components way to make sure the StyledIconsButton has the correct padding for the components that need different paddings.

abotteram · 2018-07-30T09:33:44Z

composites/Plugin/Shared/components/Button.js

+		prefixIconColor,
+		suffixIcon,
+		suffixIconColor,
+		prefixIconViewBox,


This shouldn't exist, can't we update the SVG icon to have a 1720x1720 viewbox as well? All these iconViewbox props are really cluttering the code. If it's not possible to change the viewbox of the icon, we should update the viewBox of the icons that require a different view box in the SvgIcon component.

afercia · 2018-07-30T13:35:17Z

Addressed the main CR points except the color names, which I'd prefer to keep as they're for now. We can revisit when we're sure these colors should be updated globally.

afercia · 2018-07-30T13:57:55Z

Note: as soon as this is merged, we should move #671 to the queue to adjust styling in the meta box.

boblinthorst · 2018-07-30T14:03:51Z

composites/Plugin/Shared/components/Collapsible.js

-	prefixIconCollapsed: PropTypes.string,
-	prefixIconColor: PropTypes.string,
-	prefixIconSize: PropTypes.string,
+	prefixIcon: PropTypes.shape( {


Perhaps declare the shape seperately, and reuse this declaration wherever it's used?

Done in snippetEditorFields's descriptionLengthProgress, for instance.

Not important enough that it has to be addressed now though.

boblinthorst · 2018-07-30T14:04:40Z

CR: ok 👍

abotteram · 2018-07-30T14:32:18Z

Acceptance 👍

afercia added 9 commits July 26, 2018 16:32

Code and CSS initial adjustments.

8b955e3

Control left and right icons independently.

69be577

CSS adjustments.

dd46987

More CSS adjustments.

a4b2161

Remove AnalysisCollapsible and use Collapsible directly.

7ae7427

Make content style agnostic and start adjusting props docs.

ad67adf

Merge branch 'develop' into 653-654-collapsible-header-icon-keyword

d4983f5

CSS adjustments.

92cf0f9

Adjust score icon vertical position.

a22ccac

abotteram suggested changes Jul 30, 2018

View reviewed changes

afercia added 3 commits July 30, 2018 12:42

Handle the SVG icons viewBox internally.

8c7d676

Avoid to use props for the heading styling and use styled-components.

2d45021

Group collapsible icon props.

e02533a

afercia mentioned this pull request Jul 30, 2018

Styling improvements for the Analysis results. #671

Closed

boblinthorst reviewed Jul 30, 2018

View reviewed changes

Removed superfluous props being passed to Button in IconButton component

a6bb014

abotteram approved these changes Jul 30, 2018

View reviewed changes

abotteram merged commit 358f419 into develop Jul 30, 2018

abotteram deleted the 653-654-collapsible-header-icon-keyword branch July 30, 2018 14:53

abotteram added this to the 4.9.0 milestone Jul 31, 2018

This was referenced Aug 2, 2018

Better fix for padding in the styled section container on the metabox Yoast/wordpress-seo#10497

Closed

Better alignment of collapsible arrows when there's a subtitle #690

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

653 654 collapsible header icon keyword #676

653 654 collapsible header icon keyword #676

afercia commented Jul 30, 2018

abotteram left a comment

abotteram Jul 30, 2018

abotteram Jul 30, 2018

abotteram Jul 30, 2018

afercia Jul 30, 2018

abotteram Jul 30, 2018

afercia Jul 30, 2018

abotteram Jul 30, 2018

abotteram Jul 30, 2018

afercia commented Jul 30, 2018

afercia commented Jul 30, 2018

boblinthorst Jul 30, 2018

boblinthorst commented Jul 30, 2018

abotteram commented Jul 30, 2018

653 654 collapsible header icon keyword #676

653 654 collapsible header icon keyword #676

Conversation

afercia commented Jul 30, 2018

Summary

Relevant technical choices:

Test instructions

abotteram 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

afercia commented Jul 30, 2018

afercia commented Jul 30, 2018

Choose a reason for hiding this comment

boblinthorst commented Jul 30, 2018

abotteram commented Jul 30, 2018