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.

Sign up for GitHub

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

Try: Reduce specificity of reset & classic styles. #32659

Merged
merged 2 commits into from
Aug 9, 2021
Merged

Try: Reduce specificity of reset & classic styles. #32659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2cb2e8
Try: Reduce specificity of reset & classic styles.
jasmussen Jun 14, 2021
54b4532
Restore specificity of classic auto margin rule.
jasmussen Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/block-library/src/reset.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
* of the editor by themes.
*/

.editor-styles-wrapper {
// We use :where to keep specificity minimal.
// https://css-tricks.com/almanac/selectors/w/where/
html :where(.editor-styles-wrapper) {
padding: 8px;

/**
Expand Down
10 changes: 6 additions & 4 deletions packages/edit-post/src/classic.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
// This needs specificity to override the editor styles.
// Provide baseline auto margin for centering blocks.
// Specificity is kept at this level as many classic themes output
// rules like figure { margin: 0; } which would break centering.
.editor-styles-wrapper .wp-block {
margin-left: auto;
margin-right: auto;
}

// Depreacted style needed for the block widths and alignments.
// for themes that don't support the new layout (theme.json)
.wp-block {
// Deprecated style needed for the block widths and alignments.
// for themes that don't support the new layout (theme.json).
html :where(.wp-block) {
max-width: $content-width;

// Provide every block with a default base margin. This margin provides a consistent spacing
Copy link
Contributor

@youknowriad youknowriad Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Noting that these margins can also get removed by figure { margin: 0; } do you think we should also move them up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd personally prefer to keep them there, as they as I recall were the most disruptive to work with, and the reduced specificity of those margins were the biggest win of this approach. It should also be less disruptive as it's vertical margins.

However I'm happy to move them up either in this PR or as a followup if it turns out to be necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

the biggest win of this approach.

I'm curious, in which situation the smaller specificity benefit classic themes here. What kind of styles classic themes apply here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking mostly of developing new classic themes, or refactoring older ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @jasmussen that it should be fine to keep these where they are. I tested in a few themes and didn't notice this causing any issues. If they did interfere with that figure rule, it'll just be the vertical margin (and may actually have no effect if there's margin on the items above + below the figure anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking Kjell. Cool, in any case it's an easy follow up which I will be keeping my eyes peeled on and ready to change if need be. I'd like to land this PR tomorrow or Monday, so there's still time to chime in here.

Expand Down