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

Add alignment support to post content block #24014

Merged
merged 3 commits into from Jul 30, 2020

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Jul 17, 2020

Description

Adds background & text color support to the post content block. It works well in the editor, but I think some front end stuff looks weird because of the theme.

Question: do we even want this? I feel like the same thing can be accomplished with a group block around post content.

I didn't add text size stuff since that can be handled inside the blocks. Should we support it in the post content block?

Let me know if anything else needs added to post content. It looks like alignment is working "ok".

How has this been tested?

Locally in edit site

Screenshots

Screen Shot 2020-07-16 at 7 25 25 PM

Types of changes

Enhancement

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.

@noahtallen noahtallen added [Type] Enhancement A suggestion for improvement. [Feature] Full Site Editing [Block] Post Content Affects the Post Content Block labels Jul 17, 2020
@noahtallen noahtallen self-assigned this Jul 17, 2020
@noahtallen noahtallen added the Needs Design Feedback Needs general design feedback. label Jul 17, 2020
@noahtallen
Copy link
Member Author

noahtallen commented Jul 17, 2020

Added "needs design feedback" because I'm unsure which block options should be supported by the post content block.

@github-actions
Copy link

github-actions bot commented Jul 17, 2020

Size Change: +3 B (0%)

Total Size: 1.16 MB

Filename Size Change
build/block-library/index.js 132 kB +3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.93 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 125 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.58 kB 0 B
build/block-library/editor.css 7.58 kB 0 B
build/block-library/style-rtl.css 7.76 kB 0 B
build/block-library/style.css 7.77 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.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.3 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 11.8 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.9 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.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.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 9.38 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.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.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.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 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.js 5.33 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.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.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@vindl
Copy link
Member

vindl commented Jul 20, 2020

Added "needs design feedback" because I'm unsure which block options should be supported by the post content block.

cc @olaolusoga and @dubielzyk for thoughts on this

@Addison-Stavlo
Copy link
Contributor

Question: do we even want this? I feel like the same thing can be accomplished with a group block around post content.
I didn't add text size stuff since that can be handled inside the blocks. Should we support it in the post content block?

Im not certain here as well. However, I could see a use for having the options here.

In the case that the blocks in the post content have no custom styles added, being able to add them to the post content block would be a neat way to give the content specific styles at the template level. 'Singular Template A' sets its post content block to have a green background, white text, and 25px font-size. 'Singular Template B' sets its post content block to have red background, black text, and 22px font-size. Etc., so the same post content could have different styles when rendered on different templates.

Contrary to this, if the blocks in the content already have specific styles applied they would overrule the post content block's styles. In which case applying styles at the post-content block level could be a bit confusing for users to figure out why certain blocks aren't following the options they are setting at the higher level. 🤔 🤷‍♀️

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

give the content specific styles at the template level

if the blocks in the content already have specific styles applied they would overrule the post content block's styles

This makes a lot of sense to me. I think, however, that there is one more thing to consider if we go down that path. After identifying and discussing a bug with nested block styling in Jacopo's site-tagline PR, it seems like this might require a non-trivial amount of work to get right.

Not saying template level styling isn't worth the effort. It's just another consideration.

@noahtallen
Copy link
Member Author

Maybe this is the sort of question to put on the backburner until global styles is around?

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

Maybe this is the sort of question to put on the backburner until global styles is around?

That makes sense. I think the changes @Addison-Stavlo and I are discussing would be outside the scope of this PR 👍

To clarify though, would we put questions on the backburner because:

  • global styles would already handle styling at the template level?
  • global styles would impact template level styling in another way?
  • another reason?

@noahtallen
Copy link
Member Author

To clarify though, would we put questions on the backburner because global styles would already handle styling at the template level?

Yeah, that was about my line of thinking. Essentially, global styles would be fixing any problems with overriding styles globally vs locally in a holistic way

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Jul 20, 2020

Edit - oops, don't mind me. I completely missed this is already there. 🤣 (I might have been checking Post Comments instead of Post Content)

Just a side note - for things that make sense in style parity here (since the tracking issue also listed 'group' block). We may want to add the hook for block alignment:

"align": [
"wide",
"full"
],

I think that makes sense for Post Content, while some of the others may be more debatable.

@noahtallen
Copy link
Member Author

We might want to restrict the alignment options too! Not sure :)

@Addison-Stavlo
Copy link
Contributor

We might want to restrict the alignment options too! Not sure :)

to just full/wide? that might make sense... im not sure why we would want to right or left align it? 🤔

@Copons
Copy link
Contributor

Copons commented Jul 21, 2020

We might want to restrict the alignment options too! Not sure :)

to just full/wide? that might make sense... im not sure why we would want to right or left align it? 🤔

Just a note that the left/right block alignment is not the same as text alignment.
Left/right block alignment basically makes the block float (the Image block is a good example).

For experience, mixing left/right block alignment with text alignment is a bit of a can of worms, and it's very hard to make it look right.

I'd say that left/right block alignment should only be used for inline elements (like Image), while block elements should rely on "layout" blocks like Columns.

@noahtallen
Copy link
Member Author

I'd say that left/right block alignment should only be used for inline elements (like Image)

🤔 how should we apply this to the post content block?

@Copons
Copy link
Contributor

Copons commented Jul 21, 2020

I'd say that left/right block alignment should only be used for inline elements (like Image)

🤔 how should we apply this to the post content block?

Do you mean something like positioning the post content beside another block (like a sidebar)?

It should be enough — and more manageable — to put content and sidebar in a Columns block, rather than float-align one against the other (and any non-floating element nearby).

@noahtallen
Copy link
Member Author

Yeah, I agree with you there! should we remove the left/right alignment options then? I think currently all alignment options are enabled.

@Copons
Copy link
Contributor

Copons commented Jul 21, 2020

Yeah, I agree with you there! should we remove the left/right alignment options then? I think currently all alignment options are enabled.

Yes, I think it would work better, unless someone has a good reason not to... 🤔

@noahtallen noahtallen force-pushed the add/color-support-post-content branch from 3a56305 to f94e2ce Compare July 21, 2020 20:16
@noahtallen
Copy link
Member Author

following suggestions from addie and copons, I've restricted the alignment options to wide & full

Copy link
Contributor

@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.

A couple things to note:

We will need to add some styles to an editor.scss / style.scss file for the link color setting to work within the editor and front-end. While that may not necessary for simple blocks like paragraph/heading, when we have these nested elements they compete with default theme styles.

Screen Shot 2020-07-21 at 6 06 38 PM

Note the text should be white as well. Maybe something similar to the styles in #24080 will fix this (and perhaps we need a wp-block-post-content classname added in index.php).


Another odd thing I noticed, with no/standard block alignment for this block within a Group block that has a width alignment there is an inconsistency between the editor and front-end (although probably unrelated to any changes here):

editor:
Screen Shot 2020-07-21 at 6 12 35 PM

In the editor the background color is applied to the expected width, but on the front-end it is applied to the width of the parent group block. 🤔

front-end:
Screen Shot 2020-07-21 at 6 12 43 PM


Lastly, the class names for block alignment 'alignfull' 'alignwide' etc. are not being added to the block on the front-end. But this is because we need to add handling for block alignment in gutenberg_experimental_apply_classnames_and_styles function in lib/blocks.php. This shouldn't be a blocker for this PR, and I plan on looking into adding handling for block alignment there tomorrow (it will help with Post Comments and other blocks as well).

@noahtallen
Copy link
Member Author

Following more suggestions, I've removed the color support. It really doesn't make a lot of sense in this context, IMO, at least until we have more fleshed out design solutions with global styles. (Additionally, you can just accomplish the same thing with a group block.)

@noahtallen
Copy link
Member Author

I'll also wait for #24122 so that I can test alignment on the front end

@vindl
Copy link
Member

vindl commented Jul 29, 2020

I'll also wait for #24122 so that I can test alignment on the front end

Just a note that #24122 has been merged so this should be ready for rebase and testing now.

@vindl vindl force-pushed the add/color-support-post-content branch from d1c2617 to d819ab0 Compare July 30, 2020 17:11
@vindl vindl changed the title Add color support to post content block Add alignment support to post content block Jul 30, 2020
@vindl vindl merged commit 6ab9b3a into master Jul 30, 2020
@vindl vindl deleted the add/color-support-post-content branch July 30, 2020 17:44
@github-actions github-actions bot added this to the Gutenberg 8.7 milestone Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Content Affects the Post Content Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Full site editing
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

5 participants