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

Remove duplicate SCSS rules, merge (consecutive) identical selectors. #14520

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

svenvanhal
Copy link
Contributor

Description

This PR removes a couple of duplicate CSS rules and merges consecutive selectors in the same file, in cases where the rules have (probably) not been separated on purpose, to improve readability and understandability, as well as slightly reducing the filesize.

How has this been tested?

This change has no side effects.

Types of changes

Refactor, non-breaking

@@ -1,5 +1,4 @@
.editor-error-boundary {
max-width: $content-width;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please advise as to which max-width to keep. Currently preserving existing behavior, $content-width appears to be too narrow for this element (cf. #8834)

Copy link
Member

@aduth aduth Apr 24, 2019

Choose a reason for hiding this comment

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

Please advise as to which max-width to keep. Currently preserving existing behavior, $content-width appears to be too narrow for this element (cf. #8834)

Since there's no history other than #8834 and the other style will have always taken effect, I expect you've made the correct assumption here.

@svenvanhal svenvanhal changed the title Remove duplicate rules, merge (consecutive) identical selectors. Remove duplicate SCSS rules, merge (consecutive) identical selectors. Mar 20, 2019
@ellatrix ellatrix added the Needs Design Feedback Needs general design feedback. label Apr 24, 2019
@aduth aduth added [Type] Code Quality Issues or PRs that relate to code quality and removed Needs Design Feedback Needs general design feedback. labels Apr 24, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

This is a great code quality improvement 👍

Out of curiosity, did you use some automated tool to identify these redundant styles? It would be nice if we could find a way to integrate this into existing build tooling to avoid similar future issues.

@@ -1,5 +1,4 @@
.editor-error-boundary {
max-width: $content-width;
Copy link
Member

@aduth aduth Apr 24, 2019

Choose a reason for hiding this comment

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

Please advise as to which max-width to keep. Currently preserving existing behavior, $content-width appears to be too narrow for this element (cf. #8834)

Since there's no history other than #8834 and the other style will have always taken effect, I expect you've made the correct assumption here.

@@ -1,12 +1,10 @@
.wp-block-media-text {
display: grid;
}
Copy link
Member

Choose a reason for hiding this comment

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

It appears this one was resolved separately in #13989, causing the merge conflict. I'll plan to resolve it.

@aduth aduth merged commit 774713c into WordPress:master Apr 24, 2019
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
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