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

Site Tagline Block #23788

Merged
merged 6 commits into from
Jul 15, 2020
Merged

Site Tagline Block #23788

merged 6 commits into from
Jul 15, 2020

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Jul 8, 2020

Description

Replaces #18241

Add a Site Tagline (aka Site Description) block for the Site Editor.
It supports colors, font size, and line height out of the box.

How has this been tested?

Tested in the Site Editor, on both Firefox 78 and Chrome 83 on macOS 10.15.5.

Screenshots

Screenshot 2020-07-13 at 18 31 46

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 8, 2020

Size Change: +220 B (0%)

Total Size: 1.14 MB

Filename Size Change
build/block-library/index.js 132 kB +226 B (0%)
build/components/index.js 199 kB -3 B (0%)
build/edit-navigation/index.js 10.8 kB -1 B
build/edit-site/index.js 16.8 kB -1 B
build/editor/index.js 45.1 kB -1 B
ℹ️ 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.39 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.67 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.js 115 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.6 kB 0 B
build/block-library/editor.css 7.59 kB 0 B
build/block-library/style-rtl.css 7.77 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.2 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 11.5 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 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 569 B 0 B
build/dom/index.js 3.23 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.6 kB 0 B
build/edit-post/style.css 5.6 kB 0 B
build/edit-site/style-rtl.css 3.04 kB 0 B
build/edit-site/style.css 3.04 kB 0 B
build/edit-widgets/index.js 9.35 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/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 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 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.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.js 5.32 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.4 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.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Comment on lines 5 to 7
"align": {
"type": "string"
}
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 you don't need to add that as an attribute but can just add it to supports. See for syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 29 to 34
<AlignmentToolbar
onChange={ ( newAlign ) =>
setAttributes( { align: newAlign } )
}
value={ align }
/>
Copy link
Contributor

@ockham ockham Jul 9, 2020

Choose a reason for hiding this comment

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

I think adding align to suppports will give you the AlignmentToolbar out of the box 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ockham The difference is that align in supports is the block alignment, while in this case it would be the text alignment.
Two major differences, off the top of my head:

  • Block alignment also (optionally) supports wide and full.
  • Block alignment left and right makes the block floating.

This said, your point makes sense in that we might want this block to support both block (as align supports) and text alignment (as textAlign attribute).
I don't see any other Core block doing the same thing, and I wonder if it's an intentional UX choice (it might be confusing for the user).
A middle ground could be to have align only support wide, full. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, thanks for clarifying.

I do think that this has the potential of being quite confusing to the user, so I wouldn't be surprised if it was a deliberate UX choice.

Do blocks that are somewhat comparable to Site Description tend to have either align or text align?

If we do go with text align, let's rename the attribute to textAlign, to make it easier to distinguish from block align.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The most comparable is arguably Site Title, which indeed only has the text alignment with the align attribute name.

(It's not a coincidence, I've mostly copied it to build the Site Description block... 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do go with text align, let's rename the attribute to textAlign, to make it easier to distinguish from block align.

I agree, but I'd love more Gutenberg folks chime in on this.
As far as I can see, only Verse uses textAlign for text alignment, while all other blocks simply use align.

@Copons Copons force-pushed the add/site-description-block branch from 5c54a58 to 50ee9d3 Compare July 13, 2020 13:21
@Copons
Copy link
Contributor Author

Copons commented Jul 13, 2020

Added support for experimental colors, font size, and line height (introduced in #23007).

Basically the Site Tagline block is almost identical to Site Title.
Differences:

  • They use different site options (obviously).
  • Site Tagline only outputs a paragraph element, whereas Site Title allows for paragraph or heading (1-6).

About the output element: I'm wondering if we might even "downgrade" it to a simple div instead of p, or give the choice.

Either way, I've chosen not to include the Site Title's level selector (to set it as heading element) following the path laid out by the Twenty themes: the last one using an h2 for the site description was Twenty Fourteen; since then it has always been either p or div.

@Copons Copons changed the title Site Description Block Site Tagline Block Jul 13, 2020
@ockham
Copy link
Contributor

ockham commented Jul 13, 2020

The icon is info, but of course I'm open to different suggestions.
For reference, the previous Site Description PR used this custom icon:
Screenshot 2020-07-08 at 16 19 53

I kinda like that, and I think it's less generic than the info icon. Let's use it maybe?

@Copons Copons force-pushed the add/site-description-block branch from a1897c4 to dc82e48 Compare July 13, 2020 17:22
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.

LGTM 👍

@Copons
Copy link
Contributor Author

Copons commented Jul 13, 2020

Thanks @ockham! I've updated the icon to use the one from the old PR.

I've also added a description keyword to the block definition.
While the block is now properly called "tagline" (user-facing label for the setting), I'd argue that WP developers are used to call it "description" (setting name used behind the scenes) instead — or at least I am! 😄

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.

I spent a lot of time learning about block.json and experimental-themes.json. Not an expert with the Gutenberg metadata interface, but as far as the code goes, everything looks reasonable.

I did notice one curious behavior that affects all blocks that can modify colors (including site-tagline). To clarify, this seems best addressed in a follow-up PR.

For any block with a background color set as a direct child of a group block, text padding isn't appropriately applied. After some digging, it looks like padding styles are being overwritten by group block css here on L51-52 and L59-60.

Screen Shot 2020-07-13 at 5 08 43 PM

Screen Shot 2020-07-13 at 4 51 09 PM

This occurs for me in chrome, firefox, and safari.

I'll make a separate issue for this in Gutenberg if that sounds good to you @Copons

@Copons
Copy link
Contributor Author

Copons commented Jul 14, 2020

I'll make a separate issue for this in Gutenberg if that sounds good to you @Copons

Sure @jeyip, go for it!
I've always had trouble figuring out the spacing of nested blocks.

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.

I'll make a separate issue for this in Gutenberg if that sounds good to you @Copons

#23937

Do you happen to know where we place these kinds of lower priority site editor specific bug tickets, Jacopo?

@Copons
Copy link
Contributor Author

Copons commented Jul 15, 2020

Do you happen to know where we place these kinds of lower priority site editor specific bug tickets, Jacopo?

@jeyip I'm not sure, let's ask @vindl! 🙂
Meanwhile I've labeled it as FSE and bug, just in case.

One thing about this kind of visual issues that I didn't think of mentioning earlier (I haven't checked the issue yet, btw): they might be caused by the theme's own editor style, in which case Gutenberg might not be at fault.

If this is the case, we have a few options. For example:

  • Check if the theme is doing it right, but Gutenberg style "gets in the way". We might want to improve Gutenberg style to be tighter, or less specific.
  • Otherwise, the theme style is incorrect, and we should open an issue in the theme repo.

For experience, styling nested blocks in the editor is a major pain, especially when we go out of our way to customize the blocks margins.
It's just incredibly hard to test for all possible combinations, and I've never found a good enough solution that satisfies all requirements.

@Copons Copons merged commit b303d24 into master Jul 15, 2020
@Copons Copons deleted the add/site-description-block branch July 15, 2020 10:48
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 15, 2020
@strarsis
Copy link
Contributor

So this feature is planned to be released with a near future Gutenberg release?

@noahtallen
Copy link
Member

(see my response on #24246)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Site Tagline Affects the Site Tagline Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants