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

Apply consistent spacing on the post visibility popover #9800

Merged
merged 2 commits into from Sep 14, 2018

Conversation

Projects
None yet
3 participants
@chrisvanpatten
Contributor

chrisvanpatten commented Sep 11, 2018

This is a small tweak to the padding/spacing on the post visibility popover.

Before:

visibility-before

After:

visibility-after

I also tweaked the class for the <fieldset> to be more consistent with the other BEM classes.

Note that @aduth had a question about the nested CSS classes on the original PR #9138 which I have not resolved here. It's a totally fair point, but should probably be part of a bigger discussion on CSS standards, as there are many cases of nested CSS rulesets (that probably don't need to be nested) around the Gutenberg code base.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

This is nicely harmonious looking. Nice pull requests. Visually I have no objections. I think the CSS looks cool too, but I also realize there are some intricacies that I haven't fully grokked, so okay to await a quick sanity check on that one. Thanks!

Contributor

jasmussen commented Sep 13, 2018

This is nicely harmonious looking. Nice pull requests. Visually I have no objections. I think the CSS looks cool too, but I also realize there are some intricacies that I haven't fully grokked, so okay to await a quick sanity check on that one. Thanks!

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 13, 2018

Contributor

Done! Thanks for the tip @jasmussen 👍

Contributor

chrisvanpatten commented Sep 13, 2018

Done! Thanks for the tip @jasmussen 👍

@aduth

aduth approved these changes Sep 14, 2018

@aduth aduth added this to the 3.9 milestone Sep 14, 2018

@chrisvanpatten chrisvanpatten merged commit 546b744 into WordPress:master Sep 14, 2018

2 checks passed

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

@chrisvanpatten chrisvanpatten deleted the TomodomoCo:tweak/post-visibility-spacing branch Sep 14, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 14, 2018

Member

I also tweaked the class for the <fieldset> to be more consistent with the other BEM classes.

For clarity's sake, it's really a correction of an invalid class name moreso than it merely for consistency.

The editor-post-visibility class name would only be valid for the singular root element of PostVisibility. Since there is no root element (it returns an array), it's not valid to apply anywhere else.

https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#naming

Member

aduth commented Sep 14, 2018

I also tweaked the class for the <fieldset> to be more consistent with the other BEM classes.

For clarity's sake, it's really a correction of an invalid class name moreso than it merely for consistency.

The editor-post-visibility class name would only be valid for the singular root element of PostVisibility. Since there is no root element (it returns an array), it's not valid to apply anywhere else.

https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#naming

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