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

chore: remove duplicate selectors and enable stylelint rule #365

Closed
wants to merge 1 commit into from

Conversation

harish-sethuraman
Copy link
Member

similar to eslint/eslint#16467

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for new-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/new-eslint/deploys/6364b2f06860430008dabd86
😎 Deploy Preview https://deploy-preview-365--new-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for ja-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/ja-eslint/deploys/6364b2f0e327b900092be225
😎 Deploy Preview https://deploy-preview-365--ja-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for es-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/es-eslint/deploys/6364b2f025572d000838c11d
😎 Deploy Preview https://deploy-preview-365--es-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for hi-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/hi-eslint/deploys/6364b2f02a46900009c34d07
😎 Deploy Preview https://deploy-preview-365--hi-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for pt-br-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/pt-br-eslint/deploys/6364b2f02b5140000964433f
😎 Deploy Preview https://deploy-preview-365--pt-br-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for zh-hans-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/zh-hans-eslint/deploys/6364b2f006c66e00088debd4
😎 Deploy Preview https://deploy-preview-365--zh-hans-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for fr-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/fr-eslint/deploys/6364b2f00183300008e1304f
😎 Deploy Preview https://deploy-preview-365--fr-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for de-eslint ready!

Name Link
🔨 Latest commit b7342d5
🔍 Latest deploy log https://app.netlify.com/sites/de-eslint/deploys/6364b2f0a84c3b0009574964
😎 Deploy Preview https://deploy-preview-365--de-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I hope this is not intentional. I would like @nzakas to review it.

@harish-sethuraman
Copy link
Member Author

harish-sethuraman commented Nov 4, 2022

This is not auto fix but a manual one. I went through selectors and put them to a diff checker and removed common ones.

Copy link
Member

@amareshsm amareshsm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

I'm not sure about a lot of these changes because the change the order in which properties are assigned and overridden. This isn't just about making sure that selectors aren't duplicated, it's also about making sure the cascade is working correctly. There are a lot of places where variables and properties are being overridden by @media blocks later, so moving properties from below @media blocks to above or vice versa actually changes the meaning of the CSS.

Please go back through and double-check that the order for these properties is correct.

@@ -1,20 +1,18 @@
/* @link https://utopia.fyi/type/calculator?c=320,16,1.125,1280,16,1.25,6,2,&s=0.75|0.5|0.25,1.5|2|3|4|6,s-l */

:root {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this needs to stay where it is due to the @media(screen) block below changing --fluid-screen. Otherwise, the order of overrides is backwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a miss from my side. Will take time to iterate upon this and fix it

@harish-sethuraman
Copy link
Member Author

Closing in favour of #421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants