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

Post excerpt block - add color, fontSize, lineHeight, and text alignment. #23945

Merged

Conversation

Addison-Stavlo
Copy link
Contributor

Description

As part of #21087 to add feature parity to FSE blocks. This adds settings (and some necessary restructuring/styles to support) text alignment and the experimental flags for color, fontSize, and lineHeight.

How has this been tested?

Tested on local docker env.

Verify settings applied and saved in the editor for colors, font size, line height, and text alignment are appropriately reflected in the editor and on the front-end.

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 Jul 14, 2020

Size Change: -1.02 MB (88%) 🏆

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.67 kB (0%)
build/block-editor/index.js 0 B -115 kB (0%)
build/block-editor/style-rtl.css 10.8 kB +30 B (0%)
build/block-editor/style.css 10.8 kB +33 B (0%)
build/block-library/editor-rtl.css 7.61 kB +9 B (0%)
build/block-library/editor.css 7.61 kB +12 B (0%)
build/block-library/index.js 0 B -132 kB (0%)
build/block-library/style-rtl.css 7.82 kB +50 B (0%)
build/block-library/style.css 7.82 kB +45 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.2 kB (0%)
build/components/index.js 0 B -199 kB (0%)
build/components/style-rtl.css 15.6 kB -232 B (1%)
build/components/style.css 15.6 kB -227 B (1%)
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 -569 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 -622 B (0%)
build/i18n/index.js 0 B -3.56 kB (0%)
build/is-shallow-equal/index.js 0 B -709 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.13 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 132 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/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

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.

For some reason, I'm not seeing the excerpt show up. I tried putting it in a singular template with a page selected. It also seems like alignment options don't show up in this case either...

Screen Shot 2020-07-14 at 9 11 56 PM

But font size and color work!

By the way, I think the failing php test is due to phpcs issues in the PHP file

@Copons
Copy link
Contributor

Copons commented Jul 15, 2020

+1 for not being able to test this.

I had the very same issue for Post Date in #23931, and solved it by explicitly using the postId from the context in useEntityProps.

E.g.

// block.json

{ "context": [ "postType", "postId" ] }
// edit.js

function Edit( { context: { postType, postId } ) {
  const [ excerpt, setExcerpt ] = useEntityProp( 'postType', postType, 'excerpt', postId );
  // ...
}

For some reason, useEntity* functions don't get the postId and postType from the context.
I'm very unfamiliar with how that kind of magic works, though, so I can't make an educated guess as to why it happens.

In #23931 this also had the advantage of simplifying the block, as it doesn't need the wrapping export anymore.

Once the block manages to receive the post attributes from the context, the new changes work as expected! ✨


There are several unrelated issues (which being unrelated shouldn't block this PR):

  • The more link doesn't seem to respect the "show on new line" on the front end.
  • The newline handling of the excerpt seems to be off when the content is rendered as placeholder (e.g. paragraphs are separated by three newlines — I'd expect just a single space, which is how paragraphs in the excerpt are supposed to be "cleaned").

Also, we could turn the block into a light wrapper block.
It was trivial enough for Post Date, but here it might be more involved, so better left for a separate PR.

Copy link
Contributor Author

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Updated so it 'works' in the context of selecting a specific post in the site editor's drop-down.

packages/block-library/src/post-excerpt/edit.js Outdated Show resolved Hide resolved
@noahtallen
Copy link
Member

Screen Shot 2020-07-15 at 9 11 46 PM

I added post excerpt to the index template. when i switched to an individual post, I got that error :/

@noahtallen
Copy link
Member

It looks like it is working for the homepage though!

Screen Shot 2020-07-15 at 9 13 23 PM

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.

The bold/italics buttons can be activated, but don't seem to do anything :/

Screen Shot 2020-07-15 at 9 14 01 PM

Outside of that, it looks like typography and color settings are working correctly! It would be nice to fix the fatal error before merging (if it's reproducible, anyways)

@@ -4,3 +4,7 @@
line-height: inherit;
}
}

.wp-block-post-excerpt.has-link-color a {
color: var(--wp--style--color--link) !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need !important ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either that or more specificity. Since there are a handful of nested elements in this block theme supplied generic link style rules overwrite our link color selection when we also have a background color applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an example of rule with higher-specificity and where does it come from? (Just to help with the reasoning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing (maybe there is a better way to deal with this), but currently I am testing the link colors with the Seedlet-Blocks theme (I can't seem to get the option to show up for 2020/2019-blocks). When a background is applied, the link color selection stops working because of this rule:

Screen Shot 2020-07-16 at 11 10 56 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, this is an area where the theme should adapt its styles. Ideally, with global styles (and the link color feature is part of global styles), the themes don't provide as much detailed CSS but it's more "managed".

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 was wondering about this as well - Theme styles being too restrictive vs. block styles not defining enough. Although, I was having trouble finding another theme where I could test link colors anymore (I remember previously testing them on one of the 2020/2019/blocks themes but they don't seem present anymore?).

Since link colors in simple blocks such as paragraphs work on Seedlet, I figured these more complex/nested element blocks should adapt their styles accordingly. After all has-background a does not seem like a very opinionated rule, and makes sense for a theme to supply a default style for links used in background areas. So perhaps blocks like this need some sort of rule to ensure that if a custom link color is applied at the block level, the nested elements are explicitly given a style rule to properly show that selection?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a Seedlet theme bug, though.
That :not( .has-background-background-color ) increases the specificity enough to override the block's link color.

If you try to remove that :not(), you'll see that the block's rule doesn't need the !important anymore.

All the same, you could leave the :not() on the theme, and add another class to the block's rule to make the color work without !important, e.g.

.wp-block-post-author.has-link-color a,
.wp-block-post-author.has-link-color.has-background a {
	color: var( --wp--style--color--link );
}

AFAIK, this happens because :not( .class ) works as if it added a .NOT-class to the selected element.
So you'd have .wp-block-post-author.has-link-color (2 "classes" of specificity) selected by the editor styles, and something like .has-background.NOT-has-background-background-color (again, 2 classes of specificity) selected by the theme styles.
Since the theme styles are loaded after the editor's, given the same specificity, they prevail.

I'm not entirely sure of the purpose of that :not( .has-background-background-color ) (defined here: editor, front end), by the way.
It basically forces a text color to all text blocks with a background, except those using the background palette color as background color.
I kinda suspect this selector could be revised in the theme, since there's already an override (editor, front end) for blocks with p as root element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very well put, thanks @Copons! Updated 😄

@Copons
Copy link
Contributor

Copons commented Jul 16, 2020

I'm not sure how to test the experimental link color. 🤔
Should I see a block control? Should I just see the block showing up with the has-link-color class? Is it a theme option?
I'm not seeing either (on Seedlet Blocks theme), but I can at least confirm that manually adding the CSS class seems to change the link color (from blue to black/gray, in Seedlet Blocks).

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 16, 2020

The bold/italics buttons can be activated, but don't seem to do anything :/

@noahtallen You need to highlight text for it to work (as its the standard richText tools). And this block is set up in a very odd way in that the preview of the post's content you pointed out is only a placeholder for the rich text excerpt field. So if you click on that text and start typing, it will start creating the custom excerpt to save for the post. This is the actual rich text that can have those styles applied. 🤷‍♀️

I'm not sure how to test the experimental link color. 🤔

@Copons it should be available in seedlet blocks. The 'read more...' section is a link, but you could add a link to the custom excerpt through rich text to test as well:

link-color

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 16, 2020

Update - 404fe64 - I tried using the Warning component to add a custom message. Maybe this makes the most sense:

Screen Shot 2020-07-16 at 1 33 14 PM

I also updated to prevent the error regardless of that early warning return. However, without returning the warning it would be in a state where the user can edit the excerpt text but nothing is dirtied and or savable as it has no corresponding entity to save against.


I added post excerpt to the index template. when i switched to an individual post, I got that error :/

@noahtallen - Gotcha. I get this error when I add it to a template in general as postType and postId are undefined.

We could return something like what you pointed out before - #23945 (review) - that was preventing us from seeing the error. Or change the text to say something like 'No post found.' ? Or is the error'd block better in this case because it is easy to tell that the block has encountered an error, as opposed to hiding it in some text that looks like a paragraph block?

Screen Shot 2020-07-16 at 12 11 21 PM

vs

Screen Shot 2020-07-16 at 12 12 55 PM

@noahtallen
Copy link
Member

You need to highlight text for it to work

Ahh that makes sense. I think I expected it to just change the style of the whole thing. (probably an area for design to give feedback, along with how it edits the excerpt inline)

@noahtallen
Copy link
Member

as it has no corresponding entity to save against.

I think in my brain, it makes the most sense to have it be a placeholder of some sort. Not sure what design would look like in that situation. But I think this would fit with any of the blocks that don't have context available (like post_content block without having a page selected as context). Instead of erroring or warning, i think it should show a placeholder thingy that helps people understand what it would look like when it has the right context. Since it's not really an actual "error" in that the block was inserted in a place where it is allowed to be

@Addison-Stavlo
Copy link
Contributor Author

I think I expected it to just change the style of the whole thing.

I expected that the first time I tried the block too. Its definitely a bit funky feeling atm.

@Copons
Copy link
Contributor

Copons commented Jul 20, 2020

This works well for me, but I'd rather have a different way to handle front end link colors that doesn't involve !important.

@@ -2,6 +2,9 @@
"name": "core/post-excerpt",
"category": "design",
"attributes": {
"align": {
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, this should most likely not be called align, but rather textAlign. I know align is how the Paragraph block does it, but this is confusing since there is also block alignment, which (if using the supports flag) uses align as its attribute name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, textAlign or alignText would be preferable (I was following the paragraph block's lead at first here). I saw the align support hook is for block alignment as well and noticed the confusion. I wonder how many other blocks have this conflicted naming convention, but we should start updating them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or would it be better to similarly update the align hook to be blockAlign or something similar?

If this is more recent than some of the blocks that use align for textAlign, then less people should be effected by the change. That is, Im assuming if we updated the paragraph block's attr name then a user who updates their Gutenberg version will have whatever alignment settings they saved lost?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a compatibility layer for changing a block attribute name? I think there is something for updating a block to a newer version without loosing settings 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a compatibility layer for changing a block attribute name? I think there is something for updating a block to a newer version without loosing settings 🤔

That would be good, at the very least for now I will update the FSE blocks that are using align for text alignment to use textAlign

Copy link
Member

Choose a reason for hiding this comment

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

There is indeed a way to easily handle (what would otherwise be backward-incompatible) changes in block attribute schemas:

https://developer.wordpress.org/block-editor/developers/block-api/block-deprecation/

A lot of core blocks already make use this, storing their deprecated versions in a deprecated.js file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated for the FSE blocks specifically - #24077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants