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

Add option to add text color to specific text inside RichText #16014

Merged
merged 1 commit into from Feb 7, 2020

Conversation

@phpbits
Copy link
Contributor

@phpbits phpbits commented Jun 6, 2019

Description

Add text color toolbar for #15899 and #13778

How has this been tested?

Tested on Gutenberg 5.8.0 and WordPress 5.2.1

Screenshots

Screen Capture on 2019-06-06 at 12-42-34

Types of changes

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.
@mapk
Copy link
Contributor

@mapk mapk commented Jun 15, 2019

This is looking sharp! I've got a few requirements before we get this merged.

I marked up, in red, everything that I think is missing right now. Please let me know if you have questions or need a more refined mockup.

Screen Shot 2019-06-14 at 4 59 54 PM

Do you think you can include these in the PR, @phpbits?

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 15, 2019

I'm a bit concerned about the growing number of buttons in the block toolbar. I feel this shouldn't go there or at least, not by default.


return (
<>
<BlockControls>
Copy link
Member

@ellatrix ellatrix Jun 17, 2019

BlockControls shouldn't be used by a format. These are rich text level controls, not block level controls.

Copy link
Contributor Author

@phpbits phpbits Jun 17, 2019

Unfortunately, it's the only way to have the button display where it should be. Without wrapping the Toolbar with BlockControls it's displaying below the block. Let me know if you have better suggestions. Thanks!

Copy link
Contributor Author

@phpbits phpbits Jun 17, 2019

Sorry, I think I can do this. Forgot for a moment that I'm adding this on core and not separate extension. Thanks!

@kjellr
Copy link
Contributor

@kjellr kjellr commented Jun 17, 2019

I'm a bit concerned about the growing number of buttons in the block toolbar. I feel this shouldn't go there or at least, not by default.

Since this is an inline-level control, it should be placed alongside the other inline controls (bold, italic, link, etc.). This would ensure that it's also pushed down into the "more" menu automatically.

This consideration also means that we should implement the popover contextually with the highlighted text, like we do with the link popover:

Screen-Shot-2019-06-17-at-7 47 18-AM

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

I'm a bit concerned about the growing number of buttons in the block toolbar. I feel this shouldn't go there or at least, not by default.

Since this is an inline-level control, it should be placed alongside the other inline controls (bold, italic, link, etc.). This would ensure that it's also pushed down into the "more" menu automatically.

This consideration also means that we should implement the popover contextually with the highlighted text, like we do with the link popover:

Screen-Shot-2019-06-17-at-7 47 18-AM

Wow! I love this UI! I'll try this one. I'll let you know how it goes. Thanks!

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 17, 2019

  • The colour button(s) should appear in the rich text tools dropdown, not in the block toolbar.
  • Worth noting that text and background colour used to be two different buttons. Not sure what is best.

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

  • The colour button(s) should appear in the rich text tools dropdown, not in the block toolbar.
  • Worth noting that text and background colour used to be two different buttons. Not sure what is best.

I've been using Dropbox Paper and the background color highlighter is available together with bold, italic and link. I'm not really sure what's the best position either and right now I'm just experimenting on my plugin to determine which will be the best. I'm hoping the design team would have better mockup that I can follow since we all have different ideas. Thanks!

@kjellr
Copy link
Contributor

@kjellr kjellr commented Jun 17, 2019

Worth noting that text and background colour used to be two different buttons. Not sure what is best.

I think it makes sense to display them together — this would theoretically allow us to include the accessibility contrast warning in the popover as well:

Screen Shot 2019-06-17 at 8 12 53 AM

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

I think it makes sense to display them together — this would theoretically allow us to include the accessibility contrast warning in the popover as well

Totally makes sense when user will be adding both colors. What if I'll just add Highlight color like what Dropbox Paper have. It's also better to have separate colors available than the defined ones. Here's how it looks like when separated :

Screen Capture on 2019-06-14 at 16-45-09 (1)

As for now, I'm just experimenting on my EditorsKit plugin based on my ideas. On here, it's better since we will surely come up with the best one.

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

Also, I think it's really important to decide the number of clicks when deciding the best position. Assuming user will select custom color, which is the best position. Right now, I don't like that I can't see the text when Top Toolbar is not active. In this issue I like what @kjellr suggested but it'll surely displayed different when colorpicker is open.

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 17, 2019

text color + background color + warning seems quite a bit of stuff it put in a popover.

@kjellr
Copy link
Contributor

@kjellr kjellr commented Jun 17, 2019

Yeah, I'm thinking that too — especially now that I realized the custom color stuff lives in a popover within that popover. 😩

So, let's separate them out into two separate buttons. We can always explore implementing the contrast warning as a snackbar notification perhaps.

Also, when we get to it, We should maybe explore including the custom color modal inside of the first modal, rather than popping it out into a second one. For instance:

59602505-e28f0100-90d4-11e9-879c-5ea3fb4c7d9c

(That's also a lot for one modal, but it's better than having two modals at least)

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

Also, when we get to it, We should maybe explore including the custom color modal inside of the first modal, rather than popping it out into a second one.

I'd love to do that but is this something that can be done with the current ColorPalette component?

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

How about adding the Text Color icon on the toolbar since it's the one available on TinyMCE, which folks are usually searching. Then add the Highlight Color on the dropdown.

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 17, 2019

@kjellr You could also completely replace the popover content with the custom colour UI when you click "custom colour" and include a back "<" button at the top.

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 17, 2019

Also wondering if the layout could be like this:

Text colour ... clear
o o o o o

Where the last o is multi-coloured to select a custom colour.

Makes is 4 rows instead of 6.

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 17, 2019

How about adding the Text Color icon on the toolbar since it's the one available on TinyMCE, which folks are usually searching. Then add the Highlight Color on the dropdown.

Why? Text colour is not something we want to encourage. Before Gutenberg is was nested in the "kitchen sink". It should go in the dropdown.

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

Makes is 4 rows instead of 6.

How about if there are more defined colors? Do I need to add limit on the displayed colors? Thanks!

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 17, 2019

Why? Text colour is not something we want to encourage. Before Gutenberg is was nested in the "kitchen sink". It should go in the dropdown.

Sure thing :) I'll add both of them on the dropdown then.

@kjellr
Copy link
Contributor

@kjellr kjellr commented Jun 17, 2019

Where the last o is multi-coloured to select a custom colour.

We had something like that originally, but changed to the text link after discussion in #5258. For now, we should follow that standard.

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Jun 19, 2019

@kjellr I've followed the UI but I'm not sure how to adjust the ColorPallette. I've pushed the changes. Text Color format is now under the dropdown as per @ellatrix's instruction. Let me know your thoughts. Thanks!

@jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jan 20, 2020

Hi, I fixed the color indicator position bug. I think this PR is ready for some tests. cc: @mapk, @obenland, @ellatrix.

@phpbits I'm sad to hear the volcano affected you :(

<>
<RichTextToolbarButton
className="format-library-text-color-button"
name={ isActive ? 'text-color' : undefined }
Copy link
Member

@gziolo gziolo Jan 21, 2020

@jorgefilipecosta - what's even more surprising is that the name and key discussed here is going to be RichText.ToolbarControls.undefined from time to time. It all needs to be further investigated.

Copy link
Contributor

@epiqueras epiqueras Feb 4, 2020

There's no Slot for RichText.ToolbarControls.undefined but we should just not render this component instead.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Feb 4, 2020

Hi @gziolo, @epiqueras, the key is not going to be RichText.ToolbarControls.undefined, because the key is the fillName and fill name has the following logic:

	let fillName = 'RichText.ToolbarControls';

	if ( name ) {
		fillName += `.${ name }`;
	}

So here when we pass undefined the key is going to be RichText.ToolbarControl.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Feb 4, 2020

We need this logic because we want to show the format on the main toolbar when there is a color set, but we want to hide it from the main toolbar when color is not set.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Feb 4, 2020

@epiqueras, @gziolo I think we may have a bug in slot fill, this case basically passes a dynamic name to the fill and maybe slot fill when the name changes does not remove previous fills. Setting a key seems to be a workaround for the issue.

Copy link
Contributor

@epiqueras epiqueras Feb 4, 2020

So here when we pass undefined the key is going to be RichText.ToolbarControl.

So it will be in the dropdown.

I think we may have a bug in slot fill, this case basically passes a dynamic name to the fill and maybe slot fill when the name changes does not remove previous fills. Setting a key seems to be a workaround for the issue.

Yes, see #16014 (comment).

Copy link
Member

@jorgefilipecosta jorgefilipecosta Feb 4, 2020

We should address the bug in slot&fill. But I think using a key is an ok workaround so we can merge this PR in WordPress 5.4. A key in this situation does not seem to cause problems. Any thoughts on this @gziolo, @epiqueras?

Copy link
Contributor

@epiqueras epiqueras Feb 4, 2020

Yes, but I’d rather have the key in the button itself to avoid this pattern from masking the need for a proper fix.

@paaljoachim
Copy link
Contributor

@paaljoachim paaljoachim commented Jan 31, 2020

It would be great to get movement on this PR as well as the associated issues/PR's.
I hope your doing alright where you are Jeffrey.
@phpbits @jorgefilipecosta @mapk

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Feb 4, 2020

@paaljoachim @jorgefilipecosta tested and seems to be working as expected.

@phpbits
Copy link
Contributor Author

@phpbits phpbits commented Feb 4, 2020

@phpbits I'm sad to hear the volcano affected you :(

Thanks man! We're all fine now and the volcano seems to be calmer now.

@jorgefilipecosta jorgefilipecosta force-pushed the add/text-level-color branch from f6fcf81 to ea4e4c1 Feb 4, 2020
name,
title,
tagName: 'span',
className: 'has-inline-color',
Copy link
Member

@ellatrix ellatrix Feb 6, 2020

I was hoping a class wouldn't be needed when we do this:
#15478

But I don't think it should block this feature. Let's work that out later.


function TextColorEdit( { value, onChange, isActive, activeAttributes } ) {
const colors = useSelect( ( select ) => {
const { getSettings } = select( 'core/block-editor' );
Copy link
Member

@ellatrix ellatrix Feb 6, 2020

Is there any way not to depend on the the block editor package?

Copy link
Member

@jorgefilipecosta jorgefilipecosta Feb 7, 2020

I tried to make at least nothing crashes if core/block-editor is not available. I guess as a follow up we may try to see if a better solution is possible.

Copy link
Member

@ellatrix ellatrix left a comment

Looks good to me. Not sure if I like the conditional placement of the button, but we can experiment with that. Also think it would be nicer to consolidate the custom color popover, but also could be adjusted later.

mapk
mapk approved these changes Feb 6, 2020
Copy link
Contributor

@mapk mapk left a comment

Really nice work!

Not sure if I like the conditional placement of the button, but we can experiment with that.

I totally agree with @ellatrix's comment. Having it appear and disappear is somewhat odd. But I just tested this out and it was working great. I'm all for getting this in and iterating later.

text-color 2020-02-06 09_12_51

@paaljoachim
Copy link
Contributor

@paaljoachim paaljoachim commented Feb 6, 2020

A lot of people will be happy with the opportunity to change the color of words..:)
I am glad we got this in!

@jorgefilipecosta jorgefilipecosta force-pushed the add/text-level-color branch 3 times, most recently from f7d1f8f to 23c7e73 Feb 7, 2020
@jorgefilipecosta jorgefilipecosta force-pushed the add/text-level-color branch from 23c7e73 to e4c915e Feb 7, 2020
@jorgefilipecosta jorgefilipecosta changed the title Add option to add text color to specifix text inside RichText editor Add option to add text color to specific text inside RichText Feb 7, 2020
@jorgefilipecosta jorgefilipecosta merged commit bc21b60 into WordPress:master Feb 7, 2020
1 check passed
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 7, 2020
@mapk
Copy link
Contributor

@mapk mapk commented Feb 7, 2020

So good to see this merged! Thanks, everyone! 🎉

@afercia
Copy link
Contributor

@afercia afercia commented Mar 3, 2020

Having it appear and disappear is somewhat odd.

Saw this new feature today and I'd totally second that. I would expect to find controls where I saw them the first time. Instead, they just disappear and appear in a new place. Not to mention that, from an accessibility perspective, the accessibility team has pointed out several times that the continuous appearing / disappearing of controls is highly confusing.

I'd definitely recommend to further iterate on this soon.

@paaljoachim
Copy link
Contributor

@paaljoachim paaljoachim commented Mar 4, 2020

A fix is in the works here: https://core.trac.wordpress.org/ticket/49568
and will be added to the next 5.4 RC.

@paaljoachim
Copy link
Contributor

@paaljoachim paaljoachim commented Mar 13, 2020

I have added additional issues. Where we can focus on one thing at a time.

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