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

Quote Block: Fix parsing a blockquote without a classname #485

Merged
merged 2 commits into from
Apr 24, 2017

Conversation

youknowriad
Copy link
Contributor

This PR fixes parsing a blockquote without a classname, and returns 1 as the default styling for quotes without classnames.

@youknowriad youknowriad added the [Feature] Blocks Overall functionality of blocks label Apr 24, 2017
@youknowriad youknowriad self-assigned this Apr 24, 2017
@youknowriad youknowriad requested review from nylen and aduth April 24, 2017 08:42
@youknowriad youknowriad changed the title Quote Block: Fix parsing a bloquote without a classname Quote Block: Fix parsing a blockquote without a classname Apr 24, 2017
@@ -20,6 +20,9 @@ registerBlock( 'core/quote', {
citation: html( 'footer' ),
style: node => {
const value = attr( 'blockquote', 'class' )( node );
if ( ! value ) {
return 1;
Copy link
Member

@aduth aduth Apr 24, 2017

Choose a reason for hiding this comment

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

With this, there's now three ways we're enforcing a default value for style.

  • Explicitly return 1 from style parsing
  • Return null from style parsing and fallback using || 1 in edit and save
  • Destructure with style = 1 default in controls.isActive

I think we ought to consolidate on one. My preference would be toward returning undefined in the style parsing and using destructuring with default value.

Copy link
Member

Choose a reason for hiding this comment

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

e.g.

Explicit undefined:

style: ( node ) => {
	const value = attr( 'blockquote', 'class' )( node );
	if ( ! value ) {
		return;
	}

	const match = value.match( /\bblocks-quote-style-(\d+)\b/ );
	if ( ! match ) {
		return;
	}

	return Number( match[ 1 ] );
}

Implicit undefined:

style: ( node ) => {
	const value = attr( 'blockquote', 'class' )( node );
	if ( value ) {
		const match = value.match( /\bblocks-quote-style-(\d+)\b/ );
		if ( match ) {
			return Number( match[ 1 ] );
		}
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, I'd go for the explicit one :)

@youknowriad youknowriad force-pushed the fix/quote-no-classname branch from f2510d6 to 9690c35 Compare April 24, 2017 14:55
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

This looks good and seems to simplify a fair bit on how we're we're using style 👍

@@ -22,7 +22,7 @@ window._wpGutenbergPost = {
'<!-- /wp:core/text -->',

'<!-- wp:core/quote -->',
'<blockquote class="blocks-quote-style-2"><p>Real artists ship.</p><footer><p><a href="http://www.folklore.org/StoryView.py?story=Real_Artists_Ship.txt">Steve Jobs, 1983</a></p></footer></blockquote>',
'<blockquote><p>Real artists ship.</p><footer><p><a href="http://www.folklore.org/StoryView.py?story=Real_Artists_Ship.txt">Steve Jobs, 1983</a></p></footer></blockquote>',
Copy link
Member

Choose a reason for hiding this comment

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

Depending on how we want the quote block to be styled, should we always enforce the existence of a class, even if it's the "default" (first) style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, but I think it's worth dropping here to avoid having this bug another time?

Copy link
Member

Choose a reason for hiding this comment

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

Probably, but I think it's worth dropping here to avoid having this bug another time?

I don't feel strongly either way. We can keep it as you have here and if it becomes a point of confusion, reintroduce later.

Copy link
Member

Choose a reason for hiding this comment

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

This would be better handled with unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants