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

Extract CSS resets to reusable mixins #14509

Merged
merged 3 commits into from
Mar 21, 2019

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Mar 19, 2019

Global CSS resets are a bad idea and the more we move away from them, the better is for reusability.
That said, the current resets we have are not very well organized and can't be reused across pages.

With the new widgets page, we need to share these resets between the editor and the widgets page. This PR extract these resets (not all of them) to two reusable mixins:

  • reset A context-agnostic reset
  • wp-admin-reset A way to prepare a WordPress Admin Page to be Gutenberg based (or JS UI based)

Testing instructions

  • Ensure the different Gutenberg UI elements (especially inputs and textareas...) are still showing as expected.

@youknowriad youknowriad self-assigned this Mar 19, 2019
@youknowriad youknowriad added the [Type] Code Quality Issues or PRs that relate to code quality label Mar 19, 2019
box-sizing: inherit;
}

select {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally for reusable components using inputs/selects... we shouldn't rely on the existence of these resets because this "reset" mixins are not included in the styles of these reusable components which means if you use a given component outside of the Gutenberg context, it may not be shown properly.

This is a separate task though that should be done step by step.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well in my testing! Just left a couple super-minor comments inline. Once we double check that post title stuff, this should be good to go. 👍

@@ -24,6 +24,7 @@
border: $border-width solid transparent;
border-left-width: 0;
border-right-width: 0;
border-radius: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing (FF, Chrome, Safari for Mac), this border-radius rule seems to be the only change that's needed in this file: if I remove the changes on L10 & L53-64, this still seems to work just fine. Are we sure we need those updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L10 is probably not needed, I'll check but the focus style is needed in the "Spotlight Mode"

}
}

input[type="radio"] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're missing some small-screen rules here: I just noticed our radio controls look a little weird on mobile. 😄 But that existed before this PR, and can be handled separately.

Screen Shot 2019-03-20 at 5 14 26 PM

@youknowriad
Copy link
Contributor Author

Can I get a ✅ here :)

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks, @youknowriad.

@youknowriad youknowriad merged commit 95642a4 into master Mar 21, 2019
@youknowriad
Copy link
Contributor Author

Thanks for the review @kjellr

@youknowriad youknowriad deleted the update/reset-css-styles-refactor branch March 21, 2019 12:40
// Stack borders on mobile.
border: $border-width solid transparent;
border-left-width: 0;
border-right-width: 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be the cause for a visual regression in the focus style of the title.

Note the lack of right border:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 PR to fix it #14572

input[type="month"],
input[type="number"],
select,
textarea {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may have caused a regression on the appearance of the "Code Editor" textarea, where the reset of .block-editor textarea in assigning font-family takes specificity priority over the code styling applied by the .editor-post-text-editor selector.

.block-editor,
// The modals are shown outside the .block-editor wrapper, they need these styles
.components-modal__frame {
@include reset;
}

(011 vs. 010)

Before After
Before After

Aside: Should .block-editor styles be applied at edit-post/src/style.scss?

@afercia
Copy link
Contributor

afercia commented Jul 31, 2019

Noting that with this change, all the rules related to inputs now apply also to the metaboxes area and to the custom fields area, see for example Yoast/wordpress-seo#13316

The reason is simple:

  • before: all the input rules were scoped to specific parts of the Gutenberg UI:
.editor-post-permalink,
.edit-post-sidebar,
.editor-post-publish-panel,
.block-editor-block-list__block,
.components-popover,
.components-modal__content 
  • after: they're in the new mixin which is scoped to .block-editor and .block-editor is the Gutenberg main container, which includes both the metaboxes and custom fields areas.

In other words, this change breaks UIs provided by plugins. It should either be communicated to plugin authors well in advance so they can adjust their CSS or (preferably) the new mixin should be better scoped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants