Skip to content
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

Update Post Author block to use __experimentalColor and __experimentalLineHeight #23044

Merged
merged 11 commits into from
Jul 20, 2020

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented Jun 9, 2020

Description

As a follow-up to #22877 this updates the Post Author block to use __experimentalColor support for font colors, link colors, and background colors/gradients.

experimentalColor

How has this been tested?

Tested between post editor, site editor, and front-end on local docker environment.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Jun 9, 2020

Size Change: -1.03 MB (89%) 🏆

Total Size: 1.15 MB

Filename Size Change
build/a11y/index.js 0 B -1.14 kB (0%)
build/annotations/index.js 0 B -3.67 kB (0%)
build/api-fetch/index.js 0 B -3.39 kB (0%)
build/autop/index.js 0 B -2.82 kB (0%)
build/blob/index.js 0 B -620 B (0%)
build/block-directory/index.js 0 B -7.63 kB (0%)
build/block-editor/index.js 0 B -124 kB (0%)
build/block-editor/style-rtl.css 10.8 kB +8 B (0%)
build/block-editor/style.css 10.8 kB +9 B (0%)
build/block-library/editor-rtl.css 7.6 kB +2 B (0%)
build/block-library/editor.css 7.6 kB +5 B (0%)
build/block-library/index.js 0 B -132 kB (0%)
build/block-library/style-rtl.css 7.78 kB +14 B (0%)
build/block-library/style.css 7.79 kB +13 B (0%)
build/block-serialization-default-parser/index.js 0 B -1.88 kB (0%)
build/block-serialization-spec-parser/index.js 0 B -3.1 kB (0%)
build/blocks/index.js 0 B -48.3 kB (0%)
build/components/index.js 0 B -200 kB (0%)
build/compose/index.js 0 B -9.67 kB (0%)
build/core-data/index.js 0 B -11.5 kB (0%)
build/data-controls/index.js 0 B -1.29 kB (0%)
build/data/index.js 0 B -8.45 kB (0%)
build/date/index.js 0 B -5.38 kB (0%)
build/deprecated/index.js 0 B -772 B (0%)
build/dom-ready/index.js 0 B -568 B (0%)
build/dom/index.js 0 B -3.23 kB (0%)
build/edit-navigation/index.js 0 B -10.8 kB (0%)
build/edit-post/index.js 0 B -304 kB (0%)
build/edit-site/index.js 0 B -16.8 kB (0%)
build/edit-site/style-rtl.css 3.06 kB +21 B (0%)
build/edit-site/style.css 3.06 kB +22 B (0%)
build/edit-widgets/index.js 0 B -9.35 kB (0%)
build/editor/index.js 0 B -45.1 kB (0%)
build/element/index.js 0 B -4.65 kB (0%)
build/escape-html/index.js 0 B -733 B (0%)
build/format-library/index.js 0 B -7.72 kB (0%)
build/hooks/index.js 0 B -2.13 kB (0%)
build/html-entities/index.js 0 B -621 B (0%)
build/i18n/index.js 0 B -3.56 kB (0%)
build/is-shallow-equal/index.js 0 B -711 B (0%)
build/keyboard-shortcuts/index.js 0 B -2.51 kB (0%)
build/keycodes/index.js 0 B -1.94 kB (0%)
build/list-reusable-blocks/index.js 0 B -3.12 kB (0%)
build/media-utils/index.js 0 B -5.32 kB (0%)
build/notices/index.js 0 B -1.79 kB (0%)
build/nux/index.js 0 B -3.4 kB (0%)
build/plugins/index.js 0 B -2.56 kB (0%)
build/primitives/index.js 0 B -1.4 kB (0%)
build/priority-queue/index.js 0 B -789 B (0%)
build/redux-routine/index.js 0 B -2.85 kB (0%)
build/rich-text/index.js 0 B -13.9 kB (0%)
build/server-side-render/index.js 0 B -2.71 kB (0%)
build/shortcode/index.js 0 B -1.7 kB (0%)
build/token-list/index.js 0 B -1.27 kB (0%)
build/url/index.js 0 B -4.06 kB (0%)
build/viewport/index.js 0 B -1.85 kB (0%)
build/warning/index.js 0 B -1.14 kB (0%)
build/wordcount/index.js 0 B -1.17 kB (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.min.js 1.14 kB 0 B
build/annotations/index.min.js 3.67 kB 0 B
build/api-fetch/index.min.js 3.43 kB 0 B
build/autop/index.min.js 2.82 kB 0 B
build/blob/index.min.js 620 B 0 B
build/block-directory/index.min.js 7.63 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/index.min.js 125 kB 0 B
build/block-library/index.min.js 131 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.min.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.min.js 3.1 kB 0 B
build/blocks/index.min.js 48.3 kB 0 B
build/components/index.min.js 200 kB 0 B
build/components/style-rtl.css 15.6 kB 0 B
build/components/style.css 15.6 kB 0 B
build/compose/index.min.js 9.68 kB 0 B
build/core-data/index.min.js 11.5 kB 0 B
build/data-controls/index.min.js 1.29 kB 0 B
build/data/index.min.js 8.45 kB 0 B
build/date/index.min.js 5.38 kB 0 B
build/deprecated/index.min.js 772 B 0 B
build/dom-ready/index.min.js 568 B 0 B
build/dom/index.min.js 3.23 kB 0 B
build/edit-navigation/index.min.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.min.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.min.js 16.8 kB 0 B
build/edit-widgets/index.min.js 9.37 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.min.js 45.3 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.min.js 4.65 kB 0 B
build/escape-html/index.min.js 733 B 0 B
build/format-library/index.min.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.min.js 2.13 kB 0 B
build/html-entities/index.min.js 621 B 0 B
build/i18n/index.min.js 3.56 kB 0 B
build/is-shallow-equal/index.min.js 711 B 0 B
build/keyboard-shortcuts/index.min.js 2.51 kB 0 B
build/keycodes/index.min.js 1.94 kB 0 B
build/list-reusable-blocks/index.min.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.min.js 5.32 kB 0 B
build/notices/index.min.js 1.79 kB 0 B
build/nux/index.min.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.min.js 2.56 kB 0 B
build/primitives/index.min.js 1.4 kB 0 B
build/priority-queue/index.min.js 789 B 0 B
build/redux-routine/index.min.js 2.85 kB 0 B
build/rich-text/index.min.js 13.9 kB 0 B
build/server-side-render/index.min.js 2.71 kB 0 B
build/shortcode/index.min.js 1.7 kB 0 B
build/token-list/index.min.js 1.27 kB 0 B
build/url/index.min.js 4.06 kB 0 B
build/viewport/index.min.js 1.85 kB 0 B
build/warning/index.min.js 1.14 kB 0 B
build/wordcount/index.min.js 1.17 kB 0 B

compressed-size-action


// If has background color.
if ( $has_custom_background_color || $has_named_background_color ) {
if ( $has_custom_background_color || $has_named_background_color || $has_named_gradient || $has_custom_gradient ) {
// Add has-background-color class.
$background_colors['css_classes'][] = 'has-background-color';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im wondering if this should just be 'has-background'. I notice that __experimentalColor applies the 'has-background' class as opposed to the 'has-background-color' class:

'has-background':
backgroundColor ||
style?.color?.background ||
( hasGradient && ( gradient || style?.color?.gradient ) ),

Im assuming either this should be changed to 'has-background', or the hook should be using 'has-background-color' ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely change this to has-background we should match __experimentalColor 💯

@@ -24,7 +24,9 @@ function post_author_build_css_colors( $attributes ) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is also a good opportunity to implement this #22872 (comment)

The idea being that you don't need to touch any backend or frontend code when applying the "useColors" flag, everything should just work.

Copy link
Contributor

@ockham ockham Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad To clarify, you're suggesting to basically get #22872 #21420 merged (with a change to make the gutenberg_apply_style_attribute watch out for the supports flag, rather than just individual styling related attributes), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think #22872 implements any of that server side support for these hooks, so I guess the suggestion is to implement the render_block filter in this PR to add that support to __experimentalColor ? ( I don't totally follow the implementation details for that yet, but this would be something that would need to be set up for __experimentalColors, __expermentalFontSize, etc. separately? )

The idea being that you don't need to touch any backend or frontend code when applying the "useColors" flag, everything should just work.

That would be great! But currently we also have to touch the backend to inject the classNames and styles for __experimentalFontSize and alignment as well. It sounds like these need to be updated in the same way? Maybe we should handle all of these being updated in a separate PR?

But I agree, it would be really nice to implement them. Having to go through this manually in index.php is definitely a pain we would like to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I meant #21420 rather than #22872, fixed my original comment now.)

That would be great! But currently we also have to touch the backend to inject the classNames and styles for __experimentalFontSize and alignment as well. It sounds like these need to be updated in the same way? Maybe we should handle all of these being updated in a separate PR?

The way I was reading Riad's comment was to have a PR to cover all those flags in a render_blocks filter that's run regardless of block type 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Addison-Stavlo I'm planning on working on this, do you want to team up to tackle the server side rendering? If we can get that done then we don't need to write any custom className and style code in any of these FSE blocks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning on using #23007 to add the code, so we have a block to test with at the same time.

Copy link
Contributor

@apeatling apeatling Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I was reading Riad's comment was to have a PR to cover all those flags in a render_blocks filter that's run regardless of block type 👍

One thing I'm unclear on is: should this function render classNames, inline block CSS, and global styles/css variables? I'm not clear on the relationship between your standard inline block CSS, and the new global styles using variables. /cc @youknowriad

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the classes/styles for this one have already been manually set-up, I wouldn't be opposed to merging this and circling back to update it after we have enabled the proper support and updated the other FSE blocks on the task list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -50 to -69
// Need font size in number form for named presets to be used in contrastCheckers.
const { fontSizes } = useSelect( ( select ) =>
select( 'core/block-editor' ).getSettings()
);
const fontSizeIndex = useMemo( () => groupBy( fontSizes, 'slug' ), [
fontSizes,
] );
const contrastCheckFontSize = useMemo(
() =>
// Custom size if set.
attributes.style?.typography?.fontSize ||
// Size of preset/named value if set.
fontSizeIndex[ attributes.fontSize ]?.[ 0 ].size ||
DEFAULT_CONTRAST_CHECK_FONT_SIZE,
[
attributes.style?.typography?.fontSize,
attributes.fontSize,
fontSizeIndex,
]
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we get the font size stuff out of the box with __experimentalFontSize? Yay! 🎉

Copy link
Contributor Author

@Addison-Stavlo Addison-Stavlo Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! Actually, a lot of that goo you highlighted I had to add when I added the __experimentalFontSize but was still using the HOC for colors and needed them to still 'play nice'. Now that we are using the flag for this color hook as well things work much better out of the box and all this gunk is no longer seems required.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overall looking good to me! I wonder if we should follow Riad's suggestion before merging this?

@Addison-Stavlo
Copy link
Contributor Author

I wonder if we should follow Riad's suggestion before merging this?

Id like to at least better understand what he is suggesting. It could make more sense to do that in a separate PR but its definitely something I would like to see in general. These flags work really well for blocks that just use a save component (paragraph, heading, etc.), you don't have to touch anything! It would be nice to not have to build these classes and styles manually when php is required.

@Addison-Stavlo Addison-Stavlo changed the title Update Post Author block to use __experimentalColor Update Post Author block to use __experimentalColor and __experimentalLineHeight Jul 14, 2020
@Addison-Stavlo
Copy link
Contributor Author

This is now updated for the changes made in #23007. Should be ready for a review again.

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2020-07-14 at 4 06 36 PM

it seems like the default font size for the byline is extremely small. is this something affected by the PR?

* @param array $attributes Navigation block attributes.
* @return array Font size CSS classes and inline styles.
*/
function post_author_build_css_font_sizes( $attributes ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how much code we're deleting here 💙

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 14, 2020

it seems like the default font size for the byline is extremely small. is this something affected by the PR?

The byline and bio sizes both scale off the selected font size for the post author (0.5em and 0.7em respectively set up in a previous PR). I had originally set up attributes and controls to set each of the 3 sections font sizes individually in #22877 - but that was before converting to the __experimentalFontSize. It was recommended to set them based on the master size for now, but if we want to set up explicit control for them in the future we will need to update these sections to be child blocks using that fontSize flag.

@noahtallen
Copy link
Member

For some reason, it seems like the post author avatar stopped displaying in the editor after merging master to this PR: (with the avatar toggled on)

Editor
Screen Shot 2020-07-14 at 8 59 38 PM

Front end
Screen Shot 2020-07-14 at 8 59 26 PM

I also found a weird bug:

  1. Add a post author block and save changes.
  2. Adjust one of the typography settings (e.g. line height) and save changes.
  3. Try to adjust the same typography setting. Note that it does not cause dirty changes.

This does not happen with color settings, just typography. It happens with either setting (line height or size) so long as it is the same one. If I go back to step 3 and modify the other typography setting, then I can save my changes.

Here is a gif of steps 2 and 3:

2020-07-14 21 02 31

@youknowriad
Copy link
Contributor

Very cool. I love this approach.

@Copons Copons self-requested a review July 15, 2020 10:52
@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 15, 2020

For some reason, it seems like the post author avatar stopped displaying in the editor after merging master to this PR: (with the avatar toggled on)

Hrm, I'm not able to repro this on its current commit or after merging master.

I also found a weird bug:

Add a post author block and save changes.
Adjust one of the typography settings (e.g. line height) and save changes.
Try to adjust the same typography setting. Note that it does not cause dirty changes.

Interesting. Im finding this to be true for other blocks as well (such as Heading and Paragraph), but only in the Site Editor. 🤔

@noahtallen
Copy link
Member

I'm not able to repro this on its current commit

It's working for me now 🤷 It looks like it takes a sec for the image URL to load in, so maybe it just never loaded earlier.

I'm finding this to be true for other blocks as well (such as Heading and Paragraph),

Uh oh :(

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do anything to add margin outside of the background color?

Screen Shot 2020-07-15 at 8 59 14 PM

I think the line-height settings disappeared 🤔

Screen Shot 2020-07-15 at 8 59 20 PM

placeholder={ __( 'Write byline …' ) }
value={ byline }
onChange={ ( value ) =>
setAttributes( { byline: value } )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine this is unrelated to the PR, but it is really weird that the byline is not persisted across all instances of the same author!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... it was set up as an attribute specific to the block for whatever reason. The bio on the other hand is per author.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting :p

@Addison-Stavlo
Copy link
Contributor Author

Do we need to do anything to add margin outside of the background color?

Im not sure what needs to be done for some of the base styles for the block. But that does seem like it needs some love.

I think the line-height settings disappeared 🤔

Ugg, I'm seeing this now too. It looks like this happened when I brought in the recent master. Running current version of master im finding this is is no longer present for other blocks that use the lineHeight such as Paragraph. Maybe some recent change/regression. @youknowriad perhaps have any ideas on what could have caused this to disappear?

@youknowriad
Copy link
Contributor

lineHeight is opt-in now instead of opt-out. That's probably the reason.

@youknowriad
Copy link
Contributor

you need add_theme_support( 'custom-line-height' ) on your theme.

@Copons
Copy link
Contributor

Copons commented Jul 20, 2020

This LGTM!

I haven't approved the PR yet as I'd like to see the !important removed here as well. 🙂
The full discussion is in #23945 (review)

@Addison-Stavlo
Copy link
Contributor Author

I haven't approved the PR yet as I'd like to see the !important removed here as well.

Done 😄 Thank you!

@Addison-Stavlo Addison-Stavlo merged commit 85bc090 into master Jul 20, 2020
@Addison-Stavlo Addison-Stavlo deleted the update/post-author-block-experimental-colors branch July 20, 2020 17:57
@github-actions github-actions bot added this to the Gutenberg 8.7 milestone Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants