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

WIP/Try: Make pullquote a variation #8821

Closed
wants to merge 2 commits into from
Closed

Conversation

jasmussen
Copy link
Contributor

This PR takes a meager stab at #5947 by copying the pullquote styles to the basic quote block and making it a variation, now titled "Elaborate". Name aside, which is up for discussion, this PR is mostly intended to get the ball rolling on eventually doing this, vs. keeping the status quo.

If we mean to move forward, aside from feedback, the actual pullquote needs to be deprecated and removed, and I'd need help doing that as I assume it's not enough to just delete the pullquote folder.

One issue already surfaced from this: the pullquote block features block level alignments, whereas the quote block features only inline level alignments (text align). That means you can float the pullquote, but not the quote. At least not yet, though presumably with a container block you'd be able to do that in the future.

What are your thoughts on how to move forward with this?

What you can do with the current pullquote block, but not in the variation:

screen shot 2018-08-10 at 11 19 16

Here's how the new variation looks:

pullquote variation

@jasmussen jasmussen added [Status] In Progress Tracking issues with work in progress Needs Design Feedback Needs general design feedback. Needs Decision Needs a decision to be actionable or relevant labels Aug 10, 2018
@jasmussen jasmussen self-assigned this Aug 10, 2018
@jasmussen jasmussen requested a review from a team August 10, 2018 09:25
@youknowriad
Copy link
Contributor

If we go this way, we need a way to deprecate a block in favor of another one.

@jasmussen
Copy link
Contributor Author

If we go this way, we need a way to deprecate a block in favor of another one.

Not suggesting the following, merely brainstorming: what if we simply removed it? Any existing pullquote would show up as — a classic block? A broken block? I seem to recall there being a ticket to handle this situation better?

@youknowriad
Copy link
Contributor

For now, they would show up as a classic block.

@jasmussen
Copy link
Contributor Author

For now, they would show up as a classic block.

In my deeply personal and non-official opinion, that would be fine, as you should be able to convert to blocks.

Only, I just tried, and for some reason that doesn't work, it just remains a classic block. Hmm. I wonder if that's relate to #5946

@sarahmonster
Copy link
Member

💯💯💯

The pullquote/quote conflict has so much potential to be confusing to users (Why are the alignments different? Why does a pullquote use an attribution? What's the difference between the two? Why do they both use the same icon?) that I think it really makes sense to combine them like this, even if we lose out on a little bit of functionality for the time being.

Pullquotes are great in a print setting, but they're kind of weird on the web, particularly in a mobile context.

One issue already surfaced from this: the pullquote block features block level alignments, whereas the quote block features only inline level alignments (text align). That means you can float the pullquote, but not the quote. At least not yet, though presumably with a container block you'd be able to do that in the future.

Stylistically, I could see there being as much argument for floating blockquotes as for pullquotes, especially when (assumption) the majority of users likely won't know the difference between the two, from a semantic and stylistic perspective. Block alignment on text blocks can also be a bit confusing, because the text remains centred regardless of how the block alignment is set—ideally you'd be able to set both block-level alignment (on the container) and text-level alignment on the text itself.

The benefits of collapsing these two blocks into one (reducing user confusion) should outweigh the potential sadface of losing that functionality here, especially if we have a way of re-introducing that functionality in a future iteration.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Aug 10, 2018

This is awesome, thanks for getting the ball rolling on this, @jasmussen! 🎉

I'd also really like to see floating options on block quotes — that's a super common use case and I like that it could apply to all quote types, not just pullquotes. I recognise that might not be possible in this PR but I think it's worth getting in shortly after this lands.

Only other point would be maybe name the new quote option "Fancy" instead of "Elaborate"? Fancy would be a touch more playful and humane.

@ZebulanStanphill
Copy link
Member

I don't know why the Pullquote has block alignment options, but if I had to guess, it is because pullquotes are often shown to the side or made wider than the rest of the post to make them stand out.

Now, technically, the Pullquote block should have used an <aside> to wrap its <blockquote>. Speaking of which, the Quote block may not need to have block alignment options since you could use a Container block to wrap it. If the Container block supported changing its HTML element to <aside>, you could create a semantically correct pullquote this way, and if the Container block supported float left, float right, center, wide, and full alignments, you could do everything you can with the Pullquote block right now.

So I do not think the Quote block itself needs block alignment options, since wrapping it in a Container block could provide the same functionality and also make more sense semantically for pullquotes.

And yeah, I think I prefer "Fancy" to "Elaborate".

@cbirdsong
Copy link

Incoming question ignorant of technical realities of block implementation:

Could an <aside> wrapper be applied automatically, if the content of the quote is elsewhere in the editor?

@jasmussen
Copy link
Contributor Author

It could, and that might make sense if this was indeed a separate block. But it can't be done if the pullquote style is just a variation.

@ZebulanStanphill
Copy link
Member

Given that you could accomplish the same thing as a dedicated Pullquote block by simply using a Quote block nested in a Container block, I see no reason to have a Pullquote block. Additionally, you could make that Quote-in-a-Container into a Reusable block that you could insert as a template. See also: #8403.

@chrisvanpatten
Copy link
Member

It seems highly unlikely a Container block will land in 5.0, so perhaps applying float controls to the block could be a good temporary solution?

That said, it's not possible to float quotes in Core right now, so maybe it's okay that doesn't land until later. (Could always be added as a 3rd party block or via a custom CSS class.)

@ZebulanStanphill
Copy link
Member

@chrisvanpatten

It seems highly unlikely a Container block will land in 5.0, so perhaps applying float controls to the block could be a good temporary solution?

It seems like a bad solution to me if it involves knowingly adding a feature only to remove it a major update later, which could adversely affect websites where the feature is used.

That said, it's not possible to float quotes in Core right now, so maybe it's okay that doesn't land until later. (Could always be added as a 3rd party block or via a custom CSS class.)

Agreed, though I would really prefer if even a barebones Container block made it into WordPress 5.0. As it turns out, the Atomic Blocks plugin has a Container block which is pretty nice, though it lacks the HTML element switcher that would be necessary for a semantically correct pullquote, and it also does not have float alignment options.

@karmatosed karmatosed self-requested a review August 23, 2018 20:09
@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Aug 23, 2018
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

Design wise approving 👍

@noisysocks
Copy link
Member

Only, I just tried, and for some reason that doesn't work, it just remains a classic block. Hmm. I wonder if that's relate to #5946

Does this work now that #6054 is merged?

@ZebulanStanphill
Copy link
Member

@noisysocks #6054 was reverted by #6501.

A second attempt at making the Quote block use nested blocks exists in #6520.

This PR takes a meager stab at #5947 by copying the pullquote styles to the basic quote block and making it a variation, now titled "Elaborate". Name aside, which is up for discussion, this PR is mostly intended to get the ball rolling on eventually doing this, vs. keeping the status quo.

If we mean to move forward, aside from feedback, the actual pullquote needs to be deprecated and removed, and I'd need help doing that as I assume it's not enough to just delete the pullquote folder.

One issue already surfaced from this: the pullquote block features block level alignments, whereas the quote block features only inline level alignments (text align). That means you can float the pullquote, but not the quote. At least not yet, though presumably with a container block you'd be able to do that in the future.

What are your thoughts on how to move forward with this?
@jasmussen
Copy link
Contributor Author

Okay, I gave this a rebase and a good squash, and I'm now calling the variation "fancy" instead.

I also, in a separete commit so we can revert it, tried simply removing the pullquote block entirely. Which now results in this:

screen shot 2018-08-24 at 10 46 21

Which I think is utterly fine. I think that's completely okay deprecation policy. However, there's an issue with converting the classic block to blocks — it simply doesn't work.

So to answer your question, @noisysocks,

Does this work now that #6054 is merged?

Yes! But I don't know why the block conversion doesn't work. Do you have any insights?

@ZebulanStanphill
Copy link
Member

@jasmussen It looks like the issue is the HTML comments. If you remove the HTML comments from inside the Classic block, it will transform just fine into a Quote block.

@ZebulanStanphill
Copy link
Member

Also, it looks like Quote blocks are missing the border they are supposed to have on the left in the Regular style variation.

@noisysocks
Copy link
Member

noisysocks commented Aug 27, 2018

Yes, that definitely looks like a bug. I suspect we might be able to address it as part of #7811? cc. @brandonpayton

It looks like it's because rawHandler is recursive in this case. It detects that there is <!-- wp: syntax in the HTML and so tries to parse it using the grammar. But since the wp:pullquote block does't exist, we're right back where we started from.

@youknowriad youknowriad modified the milestones: 3.7, 3.8 Aug 27, 2018
@brandonpayton
Copy link
Member

When we deprecated the Text Columns block, we left it in place so existing instances could be edited but removed it from being insertable. This seemed like a nice, gentle way to do it, though there unfortunately isn't a user-facing deprecation notice, just a console warning.

Today, (as others have said) an unregistered block will be converted to the classic block. If we land #8274, users will see a notice about the unregistered block with an invitation to leave it as-is, remove it, or convert to HTML. Once converted to a Custom HTML block, converting the pullquote html to blocks, results in the creation of a quote block with the right quotation but missing the citation. In a future iteration of the unregistered block warning, it would be nice to allow users to convert to other block types that support transforming from the unregistered type.

I hope this helps.

@ZebulanStanphill
Copy link
Member

@brandonpayton Notably, I added user-facing deprecation notices for the Pullquote block via the title and description. See #9358.

I could make a PR doing the same for the Pullquote block, but I can not really provide a good recommendation in the description for what block to use instead, nor can I add a semantically correct transform. A semantically correct pullquote uses a <blockquote> nested in an <aside>. But there is no way to have a block nested in an <aside> and have it still be a block, because there is no Container block.

As it is, I think the pullquote is in a weird sort of state where it is the only way (ignoring the Custom HTML and Classic blocks) to create a semantically correct pullquote... except that it does not even do that (it does not currently use an <aside>. So on the one hand, it does not even do its job right itself, but on the other hand, nothing else could do its job right except a Quote block nested in a Container block.

@mtias
Copy link
Member

mtias commented Sep 3, 2018

Taking a step back a bit here. I don't think Pullquotes should be represented as style variations of Quote. They are not the same entity. They have semantic value (even if the markup doesn't show it now) and a specific meaning separate from regular quotes.

The fact Pullquotes have extra block alignments (wide, full-width, left and right) should already indicate that they are different blocks. It doesn't make sense for Quote to absorb this complexity. To me this is a perfect example of something that becomes simpler if it's a separate block.

I do agree that Pullquote should be updated to be wrapped with an aside. The block itself could have style variations as well. An example from "I Love Typography":

image

image

Edit: We could include color options for blockquotes, for example, which would be a bit overkill for Quote.

jasmussen pushed a commit that referenced this pull request Sep 4, 2018
This is an alternative, per discussion, to #8821.

It adds some semantic value to the pullquote, indicating that it is intended to be separate content.

Note that we should consider doing a few other enhancements to the pullquote, if we intend to keep it:

- Let's add a transformation from Quote to Pullquote and back
- For some reason, the alignment values are not output in the markup in the editor, only on the frontend, which means the max-width we apply does not affect the content.

I could use help with both of these. I also imagine the deprecation handler needs a little extra now. @gziolo do you have bandwidth?
@jasmussen
Copy link
Contributor Author

One implication that resonates with me from your comment, Matías, is that it's every easy for us to be blindsided by the fact that using containers and child blocks we can accomplish the same with individual blocks. This does not necessarily mean we should, especially at the cost of overloading the variation interface. Sometimes a dedicated block might be the better option.

For that reason I took a new stab in #9599, and closing this one.

@jasmussen jasmussen closed this Sep 4, 2018
@jasmussen jasmussen deleted the try/pullquote-variation branch September 4, 2018 07:07
@mtias
Copy link
Member

mtias commented Sep 4, 2018

This does not necessarily mean we should, especially at the cost of overloading the variation interface. Sometimes a dedicated block might be the better option.

Yes, exactly. Also, even if similar shapes can be achieved with containers, it might not be the most intuitive to the user, and we might also lose a bit of semantic clarity. I now see Pullquote in a similar way to the Half-Media block that is being worked on.

jorgefilipecosta pushed a commit that referenced this pull request Sep 10, 2018
This is an alternative, per discussion, to #8821.

It adds some semantic value to the pullquote, indicating that it is intended to be separate content.

Note that we should consider doing a few other enhancements to the pullquote, if we intend to keep it:

- Let's add a transformation from Quote to Pullquote and back
- For some reason, the alignment values are not output in the markup in the editor, only on the frontend, which means the max-width we apply does not affect the content.

I could use help with both of these. I also imagine the deprecation handler needs a little extra now. @gziolo do you have bandwidth?
jorgefilipecosta pushed a commit that referenced this pull request Sep 11, 2018
Squashed commits:
[10feb74] Added deprecation artifacts
[d71a0cc] Fix for frontend.
[680c767] Figure.
[efdc070] Add aside wrapper to pullquote

This is an alternative, per discussion, to #8821.

It adds some semantic value to the pullquote, indicating that it is intended to be separate content.

Note that we should consider doing a few other enhancements to the pullquote, if we intend to keep it:

- Let's add a transformation from Quote to Pullquote and back
- For some reason, the alignment values are not output in the markup in the editor, only on the frontend, which means the max-width we apply does not affect the content.

I could use help with both of these. I also imagine the deprecation handler needs a little extra now. @gziolo do you have bandwidth?
jorgefilipecosta pushed a commit that referenced this pull request Sep 11, 2018
This is an alternative, per discussion, to #8821.

It adds some semantic value to the pullquote, indicating that it is intended to be separate content.

Note that we should consider doing a few other enhancements to the pullquote, if we intend to keep it:

- Let's add a transformation from Quote to Pullquote and back
- For some reason, the alignment values are not output in the markup in the editor, only on the frontend, which means the max-width we apply does not affect the content.

I could use help with both of these. I also imagine the deprecation handler needs a little extra now. @gziolo do you have bandwidth?
jasmussen added a commit that referenced this pull request Sep 12, 2018
* Add aside wrapper to pullquote

This is an alternative, per discussion, to #8821.

It adds some semantic value to the pullquote, indicating that it is intended to be separate content.

Note that we should consider doing a few other enhancements to the pullquote, if we intend to keep it:

- Let's add a transformation from Quote to Pullquote and back
- For some reason, the alignment values are not output in the markup in the editor, only on the frontend, which means the max-width we apply does not affect the content.

I could use help with both of these. I also imagine the deprecation handler needs a little extra now. @gziolo do you have bandwidth?

* Figure.

* Fix for frontend.

* Added deprecation artifacts

* Updated test fixtures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Decision Needs a decision to be actionable or relevant [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants