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

Reduce paragraph block CSS specificity #13025

Merged
merged 4 commits into from
Feb 11, 2019
Merged

Reduce paragraph block CSS specificity #13025

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fbcb42b
move dropcap focus to editor.css. Show hint of DC on focus
m-e-h Dec 19, 2018
b23a3e4
Move dropcap focus to editor.css. Reduce specificity
m-e-h Dec 19, 2018
b851085
Merge remote-tracking branch 'upstream/master' into fix/dropcap-visua…
m-e-h Feb 8, 2019
8fda064
Easing up on paragraph block specificity.
m-e-h Feb 8, 2019
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
66 changes: 32 additions & 34 deletions packages/block-library/src/paragraph/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,44 @@
p {
&.is-small-text {
font-size: 14px;
}
.is-small-text {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this selector a bit too generic? What about other blocks that use a class with this same name? Shouldn't this class be either namespaced (e.g. .wp-block-paragraph__is-small-text or changed to something along the lines of .wp-block-paragraph.is-small-text?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably should've been .wp-block-paragraph__is-small-text.
In the case of this PR though, does the p qualifier really make it any less generic?

I think .wp-block-paragraph.is-small-text is too specific for a core style. Especially with something like font-size which is likely one of the first things that a theme will override.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the record though, I would prefer .wp-block-paragraph.is-small-text over p.is-small-text. Since there are more appropriate hooks in place for themes to override the Paragraph Block font-sizes.

As for being generic, the one that sticks out to me here is .has-background. That's not even "text" specific and we're adding some hefty padding here. Is that class used in any other blocks?

Copy link
Member Author

@m-e-h m-e-h Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Dang! This is going to have to be fixed. The Button block also uses .has-background and is getting the padding from the Paragraph block CSS. Guess We're going to have to increase specificity for the paragraph block rather than decrease.

@kjellr How should I go about fixing? A new PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, good catch. I missed that selector. Open up a new PR and I'll take a look shortly.

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 the has-background and has-text-color selectors are the only ones that definitely should be changed for now. I don't believe the text size and dropcap ones are used elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Had a few minutes, so I opened #13821 to take care of this. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @kjellr!
Totally my fault. The PR was originally for the Dropcap and sort of morphed into being about specificity. I didn't put as much thought into it as I should have.

I agree that the other selectors don't look as problematic as has-background. Even if something like is-small-text is used in other blocks, we're only setting the font size, which would be expected for such a class name and would likely be the same or overridden by the other block.

It's the padding on a generic "background" selector that made this troublesome.
It could be debated that the padding style is more appropriate in theme.css. But that's a topic for another day. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

You're all amazing. Thanks for the hard work here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally my fault. The PR was originally for the Dropcap and sort of morphed into being about specificity. I didn't put as much thought into it as I should have.

No problem at all! It's all set now. Thanks again for your contribution, and thanks @ZebulanStanphill for starting the discussion that led to us catching this. 👍

font-size: 14px;
}

&.is-regular-text {
font-size: 16px;
}
.is-regular-text {
font-size: 16px;
}

&.is-large-text {
font-size: 36px;
}
.is-large-text {
font-size: 36px;
}

&.is-larger-text {
font-size: 48px;
}
.is-larger-text {
font-size: 48px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right file for these stylesheets? If I remember properly, these are theme options + editor styles? shouldn't we move these styles to the build-int editor-styles.css instead?


// Don't show the drop cap when editing the paragraph's content. It causes a
// number of bugs in combination with `contenteditable` fields. The caret
// cannot be set around it, caret position calculation fails in Chrome, and
// typing at the end of the paragraph doesn't work.
&.has-drop-cap:not(:focus)::first-letter {
float: left;
font-size: 8.4em;
line-height: 0.68;
font-weight: 100;
margin: 0.05em 0.1em 0 0;
text-transform: uppercase;
font-style: normal;
}
// Don't show the drop cap when editing the paragraph's content. It causes a
// number of bugs in combination with `contenteditable` fields. The caret
// cannot be set around it, caret position calculation fails in Chrome, and
// typing at the end of the paragraph doesn't work.
.has-drop-cap:not(:focus)::first-letter {
float: left;
font-size: 8.4em;
line-height: 0.68;
font-weight: 100;
margin: 0.05em 0.1em 0 0;
text-transform: uppercase;
font-style: normal;
}

&.has-drop-cap:not(:focus)::after {
content: "";
display: table;
clear: both;
padding-top: $block-padding;
}
.has-drop-cap:not(:focus)::after {
content: "";
display: table;
clear: both;
padding-top: $block-padding;
}

p.has-background {
.has-background {
padding: 20px 30px;
}

p.has-text-color a {
.has-text-color a {
color: inherit;
}