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

Jump to bottom

Fix stylelint errors in scss files #32386

Merged
merged 12 commits into from
Apr 18, 2019
Merged

Fix stylelint errors in scss files #32386

merged 12 commits into from
Apr 18, 2019

Conversation

simison
Copy link
Member

@simison simison commented Apr 18, 2019 •
edited

Changes proposed in this Pull Request

  • Fix some stylelint errors manually
  • Run stylelint --fix
  • Disable selector-max-id rule in few files where IDs were used extensively (mostly stuff like Calypso's #secondary/#footer or TinyMCE IDs)

Testing instructions

  • Run npm run lint:css and confirm no more errors
  • Calypso still looks great :-)

Related

@simison simison added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build [Type] Janitorial labels Apr 18, 2019
@simison simison requested a review from a team April 18, 2019 10:26
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/stylelint-errors

@matticbot
Copy link
Contributor

matticbot commented Apr 18, 2019 •
edited

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime
webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

name      parsed_size    gzip_size
manifest         +0 B        +14 B  (+0.0%)

Legacy SCSS Stylesheet
The monolithic CSS stylesheet that is downloaded on every app load.
👍 Thanks for making the stylesheet smaller in this PR!

name       parsed_size           gzip_size
style.css        -38 B  (-0.0%)      -12 B  (-0.0%)

Sections
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

name         parsed_size    gzip_size
woocommerce         +0 B         +0 B
stats               +0 B         +0 B
signup              +0 B         +0 B
sharing             +0 B         +0 B
reader              +0 B         +0 B
post-editor         +0 B         +0 B
help                +0 B         +0 B
activity            +0 B         +0 B

Async-loaded Components
React components that are loaded lazily, when a certain part of UI is displayed for the first time.

name                                         parsed_size    gzip_size
async-load-notifications                            +0 B         +0 B
async-load-design-playground                        +0 B         +0 B
async-load-design-blocks                            +0 B         +0 B
async-load-design                                   +0 B         +0 B
async-load-components-web-preview-component         +0 B         +0 B
async-load-blocks-reader-full-post                  +0 B         +0 B

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@simison simison mentioned this pull request Apr 18, 2019
Closed
@simison
Copy link
Member Author

simison commented Apr 18, 2019

Rebased and dropped the Prettier commit 👍

jsnajdr
jsnajdr approved these changes Apr 18, 2019
View reviewed changes
Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

Thanks for a nice cleanup! 👍

@jsnajdr jsnajdr mentioned this pull request Apr 18, 2019
Closed
@simison
Copy link
Member Author

simison commented Apr 18, 2019 •
edited

ci/circleci: test-e2e-canary-ie tests have been failing in other PRs today as well; failing signup tests seem unrelated.

scruffian
scruffian reviewed Apr 18, 2019
View reviewed changes
client/signup/processing-screen/style.scss
@@ -322,7 +322,7 @@
}

.signup-processing-screen__loader,
.signup-processing-screen__loader:after {
.signup-processing-screen__loader::after {
Copy link
Member

Choose a reason for hiding this comment

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

This may change the behaviour as :after might not always work. Did you test it?

Copy link
Member

Choose a reason for hiding this comment

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

Actually you can ignore this:

Browsers also accept :after, introduced in CSS2.

Copy link
Member

Choose a reason for hiding this comment

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

@scruffian When does :after and ::after behave differently? MDN says that the single colon is a CSS2 thing, two colons got introduced in CSS3. My guess would be that the difference was an issue in 2005.

scruffian
scruffian reviewed Apr 18, 2019
View reviewed changes
client/notifications/src/panel/boot/stylesheets/main.scss
left: 0px;
right: 0px;
left: 0;
right: 0;
box-shadow: -3px 1px 10px -2px rgba( var( --color-neutral-700-rgb ), 0.075 );

@media only screen and ( min-width: 480px ) {
Copy link
Member

Choose a reason for hiding this comment

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

Sidenote: it would be good to lint for these and suggest to replace with the breakpoint mixin

scruffian
scruffian approved these changes Apr 18, 2019
View reviewed changes
Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

🚢

@simison simison merged commit 68c6ac0 into master Apr 18, 2019
@simison simison deleted the fix/stylelint-errors branch April 18, 2019 13:10
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsnajdr jsnajdr jsnajdr approved these changes

@scruffian scruffian scruffian approved these changes

Assignees
No one assigned
Labels
Build [Type] Janitorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@simison @matticbot @scruffian @jsnajdr