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

Redesign Table Block Placeholder #15903

Merged
merged 7 commits into from Jun 4, 2019

Conversation

@tellthemachines
Copy link
Contributor

commented May 30, 2019

Description

Fixes #15820

Updated table block to use Placeholder component and implement design from #15820.

How has this been tested?

Updated e2e tests to reflect changed button text.

Steps to test:

  1. Create a new post;
  2. Add table block;
  3. Verify that placeholder displays according to design spec.

Screenshots

Screen Shot 2019-05-30 at 5 15 21 pm

Types of changes

Enhancement (non-breaking change which adds functionality)

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.
@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Hi @mapk and @kjellr the design changes to the table block placeholder are ready for review. As discussed on the issue, these changes vary slightly from the designs on the issue. Let me know what you think!

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Thanks, @tellthemachines! This looks great.

Here's a screenshot:

Screen Shot 2019-05-30 at 2 24 09 PM

I don't mind the labels-on-top, but I do think the number fields are a little long considering the content that'll go inside them. I wonder if we can narrow them down a bit? Something like this, perhaps:

Screen Shot 2019-05-30 at 2 23 52 PM

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@kjellr done!

Screen Shot 2019-05-31 at 2 07 03 pm

@noisysocks
Copy link
Member

left a comment

Nice work, and congrats on your first PR! 🎉🌈

Tested this locally and it's looking groovy. Just a few minor comments.

Show resolved Hide resolved packages/block-library/src/table/edit.js Outdated
Show resolved Hide resolved packages/components/src/placeholder/index.js Outdated
Show resolved Hide resolved packages/e2e-tests/specs/blocks/table.test.js Outdated
Show resolved Hide resolved packages/components/src/placeholder/README.md Outdated
Show resolved Hide resolved packages/components/src/placeholder/README.md Outdated
@@ -54,6 +54,11 @@
}
}

.components-placeholder__fieldset.is-column-layout,
.components-placeholder__fieldset.is-column-layout form {

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 31, 2019

Member

Any thoughts on overriding this style in packages/block-library/src/table/editor.scss as opposed to introducing a new prop? Will the prop be useful in any other blocks?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines May 31, 2019

Author Contributor

Not sure. The existing blocks all have a horizontal layout, but so far there are no others with multiple inputs. That is probably the scenario where the column layout is most useful. Is it likely there will be more of these in the future?

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jun 3, 2019

Member

I'd lean towards You Ain't Gonna Need It, but no strong preference either way 🙂

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jun 3, 2019

Author Contributor

Hmm good point, I'll look into a table-specific override.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jun 3, 2019

Author Contributor

OK so there's no way of doing this without changing the Placeholder component in some way, because the CSS for column layout needs to be added to an inner element, not to the wrapper that takes the className prop. The alternative to isColumnLayout would be to pass in a second className prop, say fieldsetClassName, to be applied to the inner div. That seems a bit messier, and less self-explanatory, so will stick with the current solution.

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jun 3, 2019

Member

We could pass in className="wp-block-table__placeholder" and then have:

.wp-block-table__placeholder .components-placeholder__fieldset,
.wp-block-table__placeholder .components-placeholder__fieldset form,
{
	flex-direction: column;
}

But, yes, as this point it's not very tidy. I'll defer to your judgement! 🙂

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jun 3, 2019

Author Contributor

I've been using CSS Modules for too long, keep forgetting that nesting classes is a thing 😭 😂

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Cool, thanks @tellthemachines. Looks good on my end:

Screen Shot 2019-05-31 at 9 14 58 AM

I'd like to just make sure @mapk gets one last look too, since he started the original ticket.

@noisysocks

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Doesn't look like the Travis CI failures are legitimate—I restarted the job for you!

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@mapk would be awesome to get your feedback on this change!

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I just tested as well. It looks great @tellthemachines! LGTM! :shipit: Thanks so much for working on this.

@mapk

mapk approved these changes Jun 4, 2019

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Excellent. Looks like we've got multiple , so I'll go ahead and merge this in. Thanks again, @tellthemachines!

@kjellr kjellr merged commit 0d60f1b into WordPress:master Jun 4, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@talldan talldan added this to the 5.9 (Gutenberg) milestone Jun 5, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Nice one @tellthemachines

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Redesign Table Block Placeholder (WordPress#15903)
* Adds placeholder with BlockIcon

* Adds column orientation for placeholder layout

* Styling improvements and copy

* Fixes failing e2e tests

* Adds documentation to Placeholder

* CSS adjustments as per design feedback

* Addresses PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.