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 another pullquote style #9763

Merged
merged 6 commits into from Sep 28, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Sep 10, 2018

The PR adds a new style variation (whose name is yet to be decided) to the Pull Quote block.

This style has an additional feature that allows users to choose a background color.

This PR depends on #9599.

The design is open for feedback maybe we can improve the look of the block.

How has this been tested?

I used the new style and converted between styles and did not notice any problem.
I checked that we are compatible with pull quotes created on the previous WordPress version e.g: by pasting the following code block in the code editor.

<!-- wp:pullquote {"align":"left"} -->
<figure class="wp-block-pullquote alignleft"><blockquote><p>AAA</p><cite>BBB</cite></blockquote></figure>
<!-- /wp:pullquote -->

Screenshots

screen shot 2018-09-12 at 19 39 36

screen shot 2018-09-12 at 19 38 57

@jorgefilipecosta jorgefilipecosta changed the base branch from master to try/pullquote-aside Sep 10, 2018

@jorgefilipecosta jorgefilipecosta changed the title from WIP Add another pullquote style to Add another pullquote style Sep 10, 2018

@jorgefilipecosta jorgefilipecosta requested review from jasmussen and mtias Sep 10, 2018

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 11, 2018

Contributor

Nice! I think you should be able to set a text color along with the background color.

Contributor

ZebulanStanphill commented Sep 11, 2018

Nice! I think you should be able to set a text color along with the background color.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 11, 2018

Contributor

I love it. I also love how since we're deciding to not make the pullquote a variation of the stock quote, we are at least adding a bunch of decorative aspects to it! My GitFu hasn't yet learned how to check out two PRs at the same time, so if we can get #9599 merged, I can help you with this PR if you need it.

This is how I'd love to see it work:

  • Default has no background color and the same text as body (currentColor). It also has the top and bottom borders.
  • As soon as you add a background color, the top and bottom borders disappear (appears to already be the case in this PR). Fonts/captions are unchanged.
  • Text color, as suggested by Zeb, needs to be customizable too, so you can ensure contrast.
  • Can we make it so the delicious contrast checker we have in place can work here as well?
Contributor

jasmussen commented Sep 11, 2018

I love it. I also love how since we're deciding to not make the pullquote a variation of the stock quote, we are at least adding a bunch of decorative aspects to it! My GitFu hasn't yet learned how to check out two PRs at the same time, so if we can get #9599 merged, I can help you with this PR if you need it.

This is how I'd love to see it work:

  • Default has no background color and the same text as body (currentColor). It also has the top and bottom borders.
  • As soon as you add a background color, the top and bottom borders disappear (appears to already be the case in this PR). Fonts/captions are unchanged.
  • Text color, as suggested by Zeb, needs to be customizable too, so you can ensure contrast.
  • Can we make it so the delicious contrast checker we have in place can work here as well?

@jorgefilipecosta jorgefilipecosta changed the base branch from try/pullquote-aside to master Sep 12, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 12, 2018

Member

Thank you for the review, I tried to mix the feedback and I updated this PR.
The block now looks like this:
screen shot 2018-09-12 at 19 39 36
screen shot 2018-09-12 at 19 38 57

Member

jorgefilipecosta commented Sep 12, 2018

Thank you for the review, I tried to mix the feedback and I updated this PR.
The block now looks like this:
screen shot 2018-09-12 at 19 39 36
screen shot 2018-09-12 at 19 38 57

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

In the last change I'm a little confused about how to change the background color. The new "main color" only styles the border for me.

I think we can actually be a bit simpler in the implementation:

  • "Main color" is renamed "Background color"
  • When no background color is chosen, there's a border top and bottom. Perhaps the top and bottom border are currentColor, so the color of both text, caption, and borders are set by the Text Color swatch.
  • As soon as you add a background color, the top and bottom borders disappear.

What do you think?

Contributor

jasmussen commented Sep 13, 2018

In the last change I'm a little confused about how to change the background color. The new "main color" only styles the border for me.

I think we can actually be a bit simpler in the implementation:

  • "Main color" is renamed "Background color"
  • When no background color is chosen, there's a border top and bottom. Perhaps the top and bottom border are currentColor, so the color of both text, caption, and borders are set by the Text Color swatch.
  • As soon as you add a background color, the top and bottom borders disappear.

What do you think?

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 13, 2018

Member

Hi @jasmussen,
I tried to follow @mtias suggestion ( I'm sorry if I missed something and did not got it right):

I was thinking we could actually try calling this attribute just "Main Color", and use it in the default version for the thick borders.

So the "Main Color" has two different behaviors, with the default style it changes the border color, on the solid color style it changes the background color.

"Main color" is renamed "Background color"
When no background color is chosen, there's a border top and bottom. Perhaps the top and bottom border are currentColor, so the color of both text, caption, and borders are set by the Text Color swatch.
As soon as you add a background color, the top and bottom borders disappear.

I also like this approach, and I'm open to both version. In this approach, we don't have an explicit style, right? The style changes when a background color is applied?
@mtias any thoughts on this?

Member

jorgefilipecosta commented Sep 13, 2018

Hi @jasmussen,
I tried to follow @mtias suggestion ( I'm sorry if I missed something and did not got it right):

I was thinking we could actually try calling this attribute just "Main Color", and use it in the default version for the thick borders.

So the "Main Color" has two different behaviors, with the default style it changes the border color, on the solid color style it changes the background color.

"Main color" is renamed "Background color"
When no background color is chosen, there's a border top and bottom. Perhaps the top and bottom border are currentColor, so the color of both text, caption, and borders are set by the Text Color swatch.
As soon as you add a background color, the top and bottom borders disappear.

I also like this approach, and I'm open to both version. In this approach, we don't have an explicit style, right? The style changes when a background color is applied?
@mtias any thoughts on this?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

Hah I see that now — okay, well, I honestly prefer the approach I suggested, but I can stomach the other appraoch too if Matías wants to drop the lead hammer 😉 — but as it is, I wasn't able to figure out how to make a solid background color, so we need a clearer UI in case we go that route.

Contributor

jasmussen commented Sep 13, 2018

Hah I see that now — okay, well, I honestly prefer the approach I suggested, but I can stomach the other appraoch too if Matías wants to drop the lead hammer 😉 — but as it is, I wasn't able to figure out how to make a solid background color, so we need a clearer UI in case we go that route.

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 13, 2018

Contributor

Is there any reason why we should not allow having a colored background for both style variations?

Contributor

ZebulanStanphill commented Sep 13, 2018

Is there any reason why we should not allow having a colored background for both style variations?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

Is there any reason why we should not allow having a colored background for both style variations?

Yes: fewer options, simplicity, ease of use.

It's always a balancing act.

Contributor

jasmussen commented Sep 13, 2018

Is there any reason why we should not allow having a colored background for both style variations?

Yes: fewer options, simplicity, ease of use.

It's always a balancing act.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 21, 2018

Member

A new feature suggested by @mtias that automatically computes the text color based on the background color if the user has not explicitly chosen a text color before was added.

Member

jorgefilipecosta commented Sep 21, 2018

A new feature suggested by @mtias that automatically computes the text color based on the background color if the user has not explicitly chosen a text color before was added.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 24, 2018

Contributor

automatically computes the text color based on the background color if the user has no explicitly chosen a text color before was added.

This sounds smart, especially if we can reuse the contrast code. But also perhaps a bit overkill. Could we use a CSS filter to achieve the same?

Contributor

jasmussen commented Sep 24, 2018

automatically computes the text color based on the background color if the user has no explicitly chosen a text color before was added.

This sounds smart, especially if we can reuse the contrast code. But also perhaps a bit overkill. Could we use a CSS filter to achieve the same?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 25, 2018

Contributor

image

Tweaked design a little bit.

Contributor

mtias commented Sep 25, 2018

image

Tweaked design a little bit.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 25, 2018

Contributor

Could we use a CSS filter to achieve the same?

How would this work?

Contributor

mtias commented Sep 25, 2018

Could we use a CSS filter to achieve the same?

How would this work?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 25, 2018

Contributor

@jasmussen I feel like the text alignment is a bit awkward in the above example. Not quite aligned with the main text, but not offset enough either.

I like the presentation here, but wonder what you think:

image

Contributor

mtias commented Sep 25, 2018

@jasmussen I feel like the text alignment is a bit awkward in the above example. Not quite aligned with the main text, but not offset enough either.

I like the presentation here, but wonder what you think:

image

@mtias mtias added this to the 4.0 milestone Sep 25, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

How would this work?

Like this. Okay that's not perfect. But it was really easy to write.

I feel like the text alignment is a bit awkward in the above example. Not quite aligned with the main text, but not offset enough either.

I think it's fine :D ship it.

Contributor

jasmussen commented Sep 25, 2018

How would this work?

Like this. Okay that's not perfect. But it was really easy to write.

I feel like the text alignment is a bit awkward in the above example. Not quite aligned with the main text, but not offset enough either.

I think it's fine :D ship it.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 27, 2018

Member

I think the feedback was applied. Thank you for the reviews! And thank you @mtias for improving the UI. cc: @karmatosed for a last check up on the UX.
We are also missing a code review before the merge.

Member

jorgefilipecosta commented Sep 27, 2018

I think the feedback was applied. Thank you for the reviews! And thank you @mtias for improving the UI. cc: @karmatosed for a last check up on the UX.
We are also missing a code review before the merge.

@jorgefilipecosta jorgefilipecosta requested review from karmatosed and WordPress/gutenberg-core Sep 27, 2018

@mtias

mtias approved these changes Sep 27, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 28, 2018

Member

Given that @mtias and @jasmussen looked at this PR I'm going to merge so it gets on the next RC and we have more time to test it.
That said if anyone finds a problem in the UX or code just comment here and I will look into the problem ASAP.
Thank you all for the help.

Member

jorgefilipecosta commented Sep 28, 2018

Given that @mtias and @jasmussen looked at this PR I'm going to merge so it gets on the next RC and we have more time to test it.
That said if anyone finds a problem in the UX or code just comment here and I will look into the problem ASAP.
Thank you all for the help.

@jorgefilipecosta jorgefilipecosta merged commit 3a210ab into master Sep 28, 2018

2 checks passed

codecov/project 48.53% (+<.01%) compared to a577d0e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the update/add-another-pullquote-style branch Sep 28, 2018

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 28, 2018

Contributor

@jorgefilipecosta I just noticed an issue with the automatic color picking: it does not work very well when you are using the Regular style variation.

image

Can the automatic color picking be turned off for the Regular style?

Contributor

ZebulanStanphill commented Sep 28, 2018

@jorgefilipecosta I just noticed an issue with the automatic color picking: it does not work very well when you are using the Regular style variation.

image

Can the automatic color picking be turned off for the Regular style?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment