Blocks: Add quote blocks style controls #482

youknowriad · 2017-04-21T11:14:42Z

In this PR I'm adding the quote styles controls.

The question here is how we provide these styles to the theme developers, should they be automatically included, or do we expect theme authors to implement these two styles on their own? This question could probably be answered separately.

aduth · 2017-04-21T14:04:13Z

blocks/library/quote/index.js

@@ -20,11 +20,21 @@ registerBlock( 'core/quote', {
 		citation: html( 'footer' )
 	},

+	controls: [ '1', '2' ].map( ( level ) => ( {


Not sure level applies as well here as it does for a heading. variant, variation, or style might be more accurate.

Minor: Do you think it's any different or better to treat '1' as a string vs. 1 as a number?

I think a string is better to avoid any confusion while parsing and serializing the attribute back.

Oh, that's a good point. Speaking of... how are we parsing the attribute?

aduth · 2017-04-21T14:04:21Z

blocks/library/quote/index.js

@@ -20,11 +20,21 @@ registerBlock( 'core/quote', {
 		citation: html( 'footer' )
 	},

+	controls: [ '1', '2' ].map( ( level ) => ( {
+		icon: 'format-quote',
+		title: wp.i18n.sprintf( wp.i18n.__( 'Quote %s' ), level ),


We should probably use %d placeholder, see also #423 (comment)

revert this change because I'm using strings

aduth · 2017-04-21T14:09:53Z

The question here is how we provide these styles to the theme developers, should they be automatically included, or do we expect theme authors to implement these two styles on their own?

Having a hard time tracking down the specific discussion, but I know this had been raised at some point.

Maybe a theme can define compatibility via add_theme_support? In which case we'd need a way to vary client-side block controls by server-side theme supports. Might tie in desire outlined in #399 to have server-side block filtering.

aduth · 2017-04-21T14:19:10Z

blocks/library/quote/index.js

+		onClick( attributes, setAttributes ) {
+			setAttributes( { style: variation } );
+		},
+		variation


Looks like we've highlighted a problem that our <Toolbar /> is tightly coupled to the idea of a heading level:

https://github.com/WordPress/gutenberg/blob/d068400/editor/components/toolbar/index.js#L24

Edit: Unless we think "level" is appropriate terminology for the subscript adjacent a control. Now that I've used the word... maybe "subscript" ?

Do you think we should rename it variation or suffix or something like that?

A shame that our blueprinting wordsmith @jasmussen is absent today 😄

I like subscript

nylen · 2017-04-21T15:12:43Z

The quote style is a number; let's treat it like a number.

I think a string is better to avoid any confusion while parsing and serializing the attribute back.

To me a better way to address this is to cast the value where it may be necessary to do so. Done in 968ec56.

I also changed Quote %d to Quote style %d because the latter makes more sense without context.

Edit: if either of these changes are not desired, feel free to revert, but they make sense to me.

aduth

Still looks like we're not ever parsing the style attribute.

Good catch - done in 02e832c.

youknowriad · 2017-04-21T16:51:30Z

Thanks @nylen for these changes, I do prefer the simpler string parsing, the 1,2 here are just identifiers for me and not really numbers but It's really minor. Let's ship it.

aduth · 2017-04-21T18:38:00Z

blocks/library/quote/index.js

+		citation: html( 'footer' ),
+		style: node => {
+			const value = attr( 'blockquote', 'class' )( node );
+			const match = value.match( /\bblocks-quote-style-(\d+)\b/ );


What happens if the class is removed from the blockquote ?

aduth · 2017-04-21T18:48:24Z

Makes me wonder at what point we start to defer to comment-serialized attributes as hints for what also exists in the markup, just to avoid complexity of matching class patterns etc.

Which leads into another question... if we did serialize the quote style in the comment, it'd have to be parsed as a string, yeah? Unless we did adopt JSON or another pattern to distinguish between strings and other types:

style as string:

<!-- wp:quote style:2 -->

style as number:

<!-- wp:quote foo:"definitely-a-string" style:2 -->

<!-- wp:quote {"foo":"definitely-a-string","style":2} -->

youknowriad · 2017-04-21T18:52:08Z

Good question @aduth I guess like HTML attributes comment attributes are strings by default. I guess everything could be represented as a string but JSON attributes could be a good alternative especially for complex attributes (object attributes).

Note that we already store the "align" attribute for the text block in the comment

aduth · 2017-04-21T18:55:08Z

We could adopt strict JSON behavior for the value, always wrapping string in ". Also resolves the need for values with spaces.

http://www.json.org/value.gif

nylen · 2017-04-27T19:17:33Z

Following up on a couple of things here...

I do prefer the simpler string parsing, the 1,2 here are just identifiers for me and not really numbers

In this case I think the right thing to do would be to make them identifiers (string names) rather than numbers. I don't have a strong preference, I just think we should decide one way or the other and then stick to that decision fully.

Makes me wonder at what point we start to defer to comment-serialized attributes as hints for what also exists in the markup, just to avoid complexity of matching class patterns etc.

I think we should try to avoid these attributes as much as possible, again in the interest of avoiding duplicated information.

Which leads into another question... if we did serialize the quote style in the comment, it'd have to be parsed as a string, yeah? Unless we did adopt JSON or another pattern to distinguish between strings and other types:

Agree that our current method of storing attributes isn't very robust. I'd like to see either HTML-style attributes name="value" or - if we know that we really need to store typed data - this option:

<!-- wp:quote {"foo":"definitely-a-string","style":2} -->

jasmussen · 2017-04-28T07:33:15Z

In this case I think the right thing to do would be to make them identifiers (string names) rather than numbers. I don't have a strong preference, I just think we should decide one way or the other and then stick to that decision fully.

I will defer to you all on what exactly to do here, as I'm sure there are intricate reasons one way or the other. To add some context, we are likely to see many other blocks as plugins, including special quote blocks perhaps with extra parameters. So when is it a new block vs. when is it a style for an existing block? I think it should be a style when it's the exact same markup and exact same surfaced parameters, and it would be lovely if someday maybe plugins could also add extra styles to existing blocks. That might be more scalable if they were numbers.

nylen · 2017-04-28T19:32:21Z

[plugins adding extra styles] might be more scalable if they were numbers.

I think the opposite is true. How do we guarantee that the numbers wouldn't conflict or change over time? It would be pretty bad to have a quote's style change depending on which plugins happened to be active at the time, for example.

Here are the pros and cons I can think of at the moment.

Numbers

Pros: Simpler mapping between data / names / toolbar icons; we don't have to try to name these things.

Cons: More difficult for plugin authors. As above, how do we guarantee that numbers don't conflict or change over time?

Names

Pros: More explicit, more opportunity for plugins to create their own styles (they should probably be namespaced by plugin name, with a standard way to add styles across blocks).

Cons: We have to name them; we have to surface the names in a user-friendly and accessible way.

Overall I think names is probably a better way to go, but this can wait a while and we need to think through what we want this functionality to be able to do. For example, anything we allow plugins to do should be namespaced.

Blocks: Add quote blocks style controls

7c3bd20

youknowriad added the [Feature] Blocks Overall functionality of blocks label Apr 21, 2017

youknowriad self-assigned this Apr 21, 2017

youknowriad requested review from jasmussen and ellatrix April 21, 2017 11:14

aduth reviewed Apr 21, 2017

View reviewed changes

Quote block: variation instead of level and more accurate placeholder

2fa1233

youknowriad force-pushed the add/quote-block-styles branch from 0cacdaf to 2fa1233 Compare April 21, 2017 14:24

youknowriad and others added 3 commits April 21, 2017 15:27

Block Toolbar: rename level to subscript

c844bd5

Use a number for quote style

968ec56

Clarify quote style message

33e7ef9

nylen approved these changes Apr 21, 2017

View reviewed changes

aduth previously requested changes Apr 21, 2017

View reviewed changes

nylen added 2 commits April 21, 2017 17:36

Parse quote style number out of saved markup

02e832c

Fix the build :|

4b8d3e4

youknowriad merged commit f75cdde into master Apr 21, 2017

youknowriad deleted the add/quote-block-styles branch April 21, 2017 16:51

aduth reviewed Apr 21, 2017

View reviewed changes

aduth mentioned this pull request May 3, 2017

Remove and prevent DOM access in block attributes matchers #624

Merged

yassine4999 mentioned this pull request Nov 15, 2023

[Snyk] Security upgrade jest-dev-server from 5.0.3 to 7.0.0 yassine4999/gutenberg#22

Open

yassine4999 mentioned this pull request Dec 27, 2023

[Snyk] Security upgrade jest-dev-server from 5.0.3 to 7.0.0 yassine4999/gutenberg#35

Open

yassine4999 mentioned this pull request Jan 5, 2024

[Snyk] Security upgrade jest-dev-server from 5.0.3 to 7.0.0 yassine4999/gutenberg#36

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Add quote blocks style controls #482

Blocks: Add quote blocks style controls #482

youknowriad commented Apr 21, 2017 •

edited

Loading

aduth Apr 21, 2017

aduth Apr 21, 2017

youknowriad Apr 21, 2017 •

edited

Loading

aduth Apr 21, 2017

aduth Apr 21, 2017

youknowriad Apr 21, 2017

aduth commented Apr 21, 2017

aduth Apr 21, 2017 •

edited

Loading

youknowriad Apr 21, 2017

aduth Apr 21, 2017

youknowriad Apr 21, 2017

nylen commented Apr 21, 2017 •

edited

Loading

aduth left a comment

youknowriad commented Apr 21, 2017

aduth Apr 21, 2017

aduth commented Apr 21, 2017

youknowriad commented Apr 21, 2017

aduth commented Apr 21, 2017

nylen commented Apr 27, 2017

jasmussen commented Apr 28, 2017

nylen commented Apr 28, 2017

Blocks: Add quote blocks style controls #482

Blocks: Add quote blocks style controls #482

Conversation

youknowriad commented Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Apr 21, 2017 • edited Loading

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

aduth commented Apr 21, 2017

aduth Apr 21, 2017 • edited Loading

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

nylen commented Apr 21, 2017 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

youknowriad commented Apr 21, 2017

Choose a reason for hiding this comment

aduth commented Apr 21, 2017

youknowriad commented Apr 21, 2017

aduth commented Apr 21, 2017

nylen commented Apr 27, 2017

jasmussen commented Apr 28, 2017

nylen commented Apr 28, 2017

Numbers

Names

youknowriad commented Apr 21, 2017 •

edited

Loading

youknowriad Apr 21, 2017 •

edited

Loading

aduth Apr 21, 2017 •

edited

Loading

nylen commented Apr 21, 2017 •

edited

Loading