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 RichText.isEmpty #9249

Merged
merged 4 commits into from Aug 30, 2018

Conversation

Projects
None yet
4 participants
@iseulde
Member

iseulde commented Aug 22, 2018

Description

Split out from #7890.

This PR introduces RichText.isEmpty as a utility function to make it easier for block authors to check if a RichText value is empty (or not), to ensure the way it is checked is consistent across all blocks, and to future-proof the API if it would change it's value shape (#7890).

How has this been tested?

Ensure all changed block still work as expected, e.g. empty captions should disappear if the block is not selected.

Screenshots

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.

@iseulde iseulde added this to the 3.7 milestone Aug 22, 2018

@iseulde iseulde requested review from youknowriad, gziolo, aduth and WordPress/gutenberg-core Aug 22, 2018

@aduth

If a block implementer is already expected to understand that a children source result takes the shape of array (by its assigned attribute type), what value is added by abstracting the part where we check that this array is empty?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 23, 2018

Member

If a block implementer is already expected to understand that a children source result takes the shape of array (by its assigned attribute type), what value is added by abstracting the part where we check that this array is empty?

In some cases, we need this check to conditionally render placeholder:

{ ( ! RichText.isEmpty( caption ) || isSelected ) && (
...
) }

However, I'm wondering if we can consolidate it inside the RichText.Content to simplify usage by providing isSelected as one of the props. In that case, I think it would override BlockEditContext.

Still, there is one place where standalone RichText.isEmpty is still valuable.

Member

gziolo commented Aug 23, 2018

If a block implementer is already expected to understand that a children source result takes the shape of array (by its assigned attribute type), what value is added by abstracting the part where we check that this array is empty?

In some cases, we need this check to conditionally render placeholder:

{ ( ! RichText.isEmpty( caption ) || isSelected ) && (
...
) }

However, I'm wondering if we can consolidate it inside the RichText.Content to simplify usage by providing isSelected as one of the props. In that case, I think it would override BlockEditContext.

Still, there is one place where standalone RichText.isEmpty is still valuable.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 24, 2018

Member

If a block implementer is already expected to understand that a children source result takes the shape of array (by its assigned attribute type), what value is added by abstracting the part where we check that this array is empty?

@aduth Well, there's no certainty that a RichText value will stay an array. I think it is good if the block implementor doesn't have to concern themselves with the shape of a RichText value, unless they're building an alternative RichText component. I'd eventually also add a rich-text type for that reason.

Another argument: currently isEmpty does not support multiline values entirely. It will check if there is a line, but not if that one line is empty.

Member

iseulde commented Aug 24, 2018

If a block implementer is already expected to understand that a children source result takes the shape of array (by its assigned attribute type), what value is added by abstracting the part where we check that this array is empty?

@aduth Well, there's no certainty that a RichText value will stay an array. I think it is good if the block implementor doesn't have to concern themselves with the shape of a RichText value, unless they're building an alternative RichText component. I'd eventually also add a rich-text type for that reason.

Another argument: currently isEmpty does not support multiline values entirely. It will check if there is a line, but not if that one line is empty.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 24, 2018

Member

@aduth Well, there's no certainty that a RichText value will stay an array.

Is it not a certainty if we're expecting block implementers to assign array as the type for the attribute?

Member

aduth commented Aug 24, 2018

@aduth Well, there's no certainty that a RichText value will stay an array.

Is it not a certainty if we're expecting block implementers to assign array as the type for the attribute?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 27, 2018

Member

I'm not sure why we're not implying the type form the source, or having it be a custom type for this source.

Member

iseulde commented Aug 27, 2018

I'm not sure why we're not implying the type form the source, or having it be a custom type for this source.

@tofumatt

The argument for relying on RichText values being of the array type doesn't address that this is simply a nicer/more expressive API, and definitely reads better than the .length checks it replaces here.

Plus, I think it's always better that we abstract away things behind an API we control.

My only comment here is I don't really like the static class method API on a component; I feel like import { isRichTextEmpty } from '@wordpress/editor' or whatever might be nicer than RichText.isEmpty… but I don't really care all that much. Now that I write that I'm not even convinced, so 🤷‍♂️

But I think this API is much nicer and gives us more control. I say 🚢

I'm just testing locally, if it's all good I'll approve this...

@tofumatt

Tested locally and worked for me across tested blocks. I say 🚢

Once
https://github.com/WordPress/gutenberg/pull/9249/files#r212015171 is addressed I think this is good.

@iseulde iseulde referenced this pull request Aug 28, 2018

Open

RichText state structure for value manipulation #7890

4 of 4 tasks complete
@tofumatt

There are places like c59958f#diff-e373e566d15f8f4196eb0b3998adbdd7R460 where I don't follow why we are doing:

const isEmpty = this.isEmpty();
isEmpty();

over just using isRichTextValueEmpty() directly.

I thought it would be nice to export isRichTextValueEmpty and use it directly but I'm not sure if that's a pain. Also it looks like we do have static class props (like RichText.Content) so I guess RichText.isEmpty has precedence as is cool.


Nevermind, I just understood why we have this.isEmpty(). But I think it would be nice to only have this.isEmpty() not take arguments and use isRichTextValueEmpty() when not checking for the current component's empty state based on this.props. Does that make sense?

So yeah, this is good. I think it would make sense for this.isEmpty( after ), this.isEmpty( before ), etc. to be changed to isRichTextValueEmpty( before ). I can push a quick commit and if it's okay we can ship that and it makes the instance isEmpty() a bit simpler of an API. I don't think we need to have it take arguments just to pass them through to isRichTextValueEmpty 🤷‍♂️

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 28, 2018

Member

I added 51f5368 to try and stop us just passing values through to the utility function. We could just nix this.isEmpty entirely but I see how it's handy so I'm fine with keeping it. If you don't think it's a helpful change feel free to revert
51f5368 and then merge… the tests are passing for me so maybe it's just Travis being weird? 😕

Member

tofumatt commented Aug 28, 2018

I added 51f5368 to try and stop us just passing values through to the utility function. We could just nix this.isEmpty entirely but I see how it's handy so I'm fine with keeping it. If you don't think it's a helpful change feel free to revert
51f5368 and then merge… the tests are passing for me so maybe it's just Travis being weird? 😕

@iseulde iseulde merged commit 7447194 into master Aug 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the try/rich-text-empty-util branch Aug 30, 2018

@iseulde iseulde referenced this pull request Sep 19, 2018

Merged

Image Block: Replace length check with RichText.isEmpty #10029

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