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

Components: Improve empty elements filters in Slot implementation #9371

Merged
merged 2 commits into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@gziolo
Member

gziolo commented Aug 27, 2018

Description

Raised by @aduth in #5952 (comment):

Why do we filter here?

To get rid of empty elements. Some fills render nothing based on Redux state. If you want to prevent rendering a slot wrapper it it has no fill which render something this is the way to go. In retrospective, I think this could be also done outside. Should we move it out or improve here and add some comments?

Comments would be good.
It was also strange to me that we filter only if the children is passed as a function, but not otherwise.
Also, filter( Boolean ) doesn't sound like a sufficient way to imitate React's treatment of "empty". For example, a child of 0 would be omitted here, but is a valid / non-empty child to render.

This PR adds isEmptyElement helper method and adds proper inline documentation explaining why it is used with fills.

How has this been tested?

npm test

Types of changes

Refactoring.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo self-assigned this Aug 27, 2018

@gziolo gziolo requested a review from aduth Aug 27, 2018

@gziolo gziolo referenced this pull request Aug 27, 2018

Merged

Editor: Bring back Inspector Advanced Controls #5952

3 of 3 tasks complete

@gziolo gziolo requested a review from WordPress/gutenberg-core Aug 28, 2018

@gziolo gziolo added this to the 3.8 milestone Aug 30, 2018

@talldan

Looks like a nice improvement. The only edge-case I can think of is isEmptyElement( new String( '' ) ), which I think would return false. It'd be pretty easy to fix that using the same approach used with arrays.

Would also be great to add jsdocs for isEmptyElement

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 5, 2018

Member

Looks like a nice improvement. The only edge-case I can think of is isEmptyElement( new String( '' ) ), which I think would return false. It'd be pretty easy to fix that using the same approach used with arrays.

Correct, I updated the way we validate strings to accommodate that. I skipped a similar check for new Boolean( false ) because it ain't as easy. In general, both should never happen 😃

Would also be great to add jsdocs for isEmptyElement

Added.

Member

gziolo commented Sep 5, 2018

Looks like a nice improvement. The only edge-case I can think of is isEmptyElement( new String( '' ) ), which I think would return false. It'd be pretty easy to fix that using the same approach used with arrays.

Correct, I updated the way we validate strings to accommodate that. I skipped a similar check for new Boolean( false ) because it ain't as easy. In general, both should never happen 😃

Would also be great to add jsdocs for isEmptyElement

Added.

@talldan

talldan approved these changes Sep 5, 2018

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 5, 2018

Contributor

Great - I'm happy with those changes.

In general, both should never happen

🤞 hopefully!

Contributor

talldan commented Sep 5, 2018

Great - I'm happy with those changes.

In general, both should never happen

🤞 hopefully!

@youknowriad youknowriad merged commit 0ea95fc into master Sep 5, 2018

2 checks passed

codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/is-empty-element branch Sep 5, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 5, 2018

Member

This pierces the abstraction of @wordpress/element. Should not this have been added there instead?

Member

aduth commented Sep 5, 2018

This pierces the abstraction of @wordpress/element. Should not this have been added there instead?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 5, 2018

Member

This pierces the abstraction of @wordpress/element. Should not this have been added there instead?

I suppose it's okay to allow this to be deferred as it relates to a 3.8 RC, only because it's not part of a public API.

Member

aduth commented Sep 5, 2018

This pierces the abstraction of @wordpress/element. Should not this have been added there instead?

I suppose it's okay to allow this to be deferred as it relates to a 3.8 RC, only because it's not part of a public API.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 6, 2018

Member

I suppose it's okay to allow this to be deferred as it relates to a 3.8 RC, only because it's not part of a public API.

I left my own comment about it #9371 (comment) where I assumed we don't want to expose it in @wordpress/element until we have another use case for it.

Member

gziolo commented Sep 6, 2018

I suppose it's okay to allow this to be deferred as it relates to a 3.8 RC, only because it's not part of a public API.

I left my own comment about it #9371 (comment) where I assumed we don't want to expose it in @wordpress/element until we have another use case for it.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 6, 2018

Member

In that case I'd rather just flag it as experimental.

My main issue is that, as far as anything in components is concerned, there should be zero awareness of what value an element holds. There is no commitment guaranteed by @wordpress/element that this shape is to remain, and will be hard to account for if ever changed. And we'd do well to avoid future headaches of plugin authors who follow similar practices to see how to accommodate this need by "solving" it internally for our own usage (as otherwise, they'll follow the same pattern and similarly face breakage whenever element structure changes).

Member

aduth commented Sep 6, 2018

In that case I'd rather just flag it as experimental.

My main issue is that, as far as anything in components is concerned, there should be zero awareness of what value an element holds. There is no commitment guaranteed by @wordpress/element that this shape is to remain, and will be hard to account for if ever changed. And we'd do well to avoid future headaches of plugin authors who follow similar practices to see how to accommodate this need by "solving" it internally for our own usage (as otherwise, they'll follow the same pattern and similarly face breakage whenever element structure changes).

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 7, 2018

Member

Opened #9681 with the proposed change. Let's continue there.

Member

gziolo commented Sep 7, 2018

Opened #9681 with the proposed change. Let's continue there.

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