Migrate stylelint to latest version #2292 #2425

LamJiuFong · 2024-02-15T15:59:49Z

What is the purpose of this pull request?
Fixes #2292

Overview of changes:
Migrate stylelint to latest version ^16.0.0
Add devdependency "stylelint-config-recommended-vue"
Add plugin @stylistic/stylelint-plugin
Add dependency postcss@^8.4.35
Fix syntax to obey new rules

Anything you'd like to highlight/discuss:
Added a comment below

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Migrate stylelint to v16.0.0

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Add dependency "stylelint-config-recommended-vue" Fix syntax to obey new rules

LamJiuFong · 2024-02-15T16:00:44Z

Hi, I am currently working on this issue locally and I would like to ask for some advices:

This is what I've done:

Update the dependencies to the latest versions and add the dependency stylelint-config-recommended-vue since stylelint doesn't support linting vue files and it is also a recommended config by stylelint.
Add stylelint-config-recommended-vue to the extension list and removed (commented out) the indentation rule since it is not supported by stylelint.
Run npm install followed by npm run csslintfix and got the errors below:

Manually fix the errors above, no errors left.
Run npm run updatetest and npm run test. Successfully ran target test.

My questions are:

Is it the right way to solve this issue by following the steps above? Did I miss out anything?
Do I need to change anything in the .github/workflows/ci.yml file? If not, is it because Github Actions will run the ci.yml in my code which uses the updated dependencies.
Some new rules are introduced and running npm run csslintfix changes the original code.
eg. border-color: rgba(150, 150, 150, 0.2) is changed to border-color: rgb(150 150 150 / 20%). Is it okay to keep the new changes? I think it should be fine as long as the syntax is standardised everywhere.
Not sure why some css files are included in .stylelintignore, could someone explain the purpose of it? Thanks!

EltonGohJH · 2024-02-16T05:59:33Z

Is it the right way to solve this issue by following the steps above? Did I miss out anything?

Looks correct on my end. For the indentation rule can you check on whether we can get a replacement?

Do I need to change anything in the .github/workflows/ci.yml file? If not, is it because Github Actions will run the ci.yml in my code which uses the updated dependencies.

I dun think so? We still run npm run test command in CI? (There is no change right?) If any change, you should change our test command.

Some new rules are introduced and running npm run csslintfix changes the original code.
eg. border-color: rgba(150, 150, 150, 0.2) is changed to border-color: rgb(150 150 150 / 20%). Is it okay o keep the new changes? I think it should be fine as long as the syntax is standardised everywhere.

I agree that if you can fix and ensure that this new style is standardised in current code base then it is good to go.

Not sure why some css files are included in .stylelintignore, could someone explain the purpose of it? Thanks!

min.css is ignored because it is minified and will not follow the standards
_site is the css in templates and should not be linted.
coverage and test should not be linted
Vue components folder also has coverage folder. Maybe you can specify just the coverage folder? I dun think our vue components folder has css files. (Double check)

codecov · 2024-02-25T08:54:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.92%. Comparing base (990d613) to head (f9ba940).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2425   +/-   ##
=======================================
  Coverage   48.92%   48.92%           
=======================================
  Files         124      124           
  Lines        5245     5245           
  Branches     1110     1110           
=======================================
  Hits         2566     2566           
  Misses       2371     2371           
  Partials      308      308

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LamJiuFong · 2024-02-25T11:49:16Z

Looks correct on my end. For the indentation rule can you check on whether we can get a replacement?

I have added a plugin @stylistic/stylelint-plugin to solve this issue as they support the deprecated indentation rule.

I dun think so? We still run npm run test command in CI? (There is no change right?) If any change, you should change our test command.

Yup, there is no change

I agree that if you can fix and ensure that this new style is standardised in current code base then it is good to go.

Ok

Vue components folder also has coverage folder. Maybe you can specify just the coverage folder? I dun think our vue components folder has css files. (Double check)

Yes, I checked and there is no css files in the vue-components folder.

Something to add on:

I ran to this error when running npm run csslint:

SyntaxError: Named export 'Input' not found. The requested module 'postcss' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'postcss';
const { rule: _rule, Input } = pkg;

This is because @stylistic/stylelint-plugin uses named export for Input and postcss only supports this starting from v8,

So, I added a dependency to specify the version of postcss to ^8.4.35 to get over this issue.

kaixin-hc · 2024-02-25T12:00:40Z

packages/core-web/src/styles/markbind.css

@@ -27,8 +27,7 @@ pre.hljs > code {
    background: none;
 }

-pre > code.hljs {


Most of these lint changes look like lint changes -> but this one confuses me, because it seems to just remove the -webkit-background-clip ? Any idea of the reason for this?

...Answered my own question: here are the links to read about it:

Stylelint removes vendor prefixes

this is a danger for specific properties... eg background-clip: text ... but this particular case doesn't seem to be an issue

Background reading on vendor prefixing: Is vendor prefixing dead?

Yup, it violates the property-no-vendor-prefix rule.

We can solve this issue by ignoring the property.

This will keep the original line
-webkit-background-clip: padding-box; /* for Safari */

I think we can do this, what do you think?

The understanding I have is that when this code was written, the browsers required prefixing for these due to not supporting the properties unprefixed. Stylelint relies on autoprefixer, an open source project that automatically adds prefixes when needed.

The implication here is that the prefixes are no longer needed due to them being supported by the browser at this point, though they were not supported in the past.

Hence, assuming there is no change in behaviour, I think we should keep the rule and change the CSS.

Oh! However do remove the comments "for safari" / references to other browsers, as these are no longer relevant.

Oh I think you are right, for now since browsers do not require vendor prefixes, I think the line background-clip: padding-box would be enough to accommodate all browsers.

I have removed the unnecessary comments.

kaixin-hc

LGTM - I don't love how many changes there are, but I suppose it is somewhat expected with a linting tool. Will wait before merging in case I've missed something.

kaixin-hc · 2024-02-25T16:05:55Z

Would you also add "Fixes #2292" to the PR description? This allows us to auto-close the issue and links the issue in the body so it easier for the reviewers!

LamJiuFong · 2024-02-26T00:11:35Z

Added!

yucheng11122017

Hi @LamJiuFong thank you very much for the good work!
One minor nit and one issue with the tooltip styling

package.json

yucheng11122017 · 2024-02-26T02:12:17Z

packages/vue-components/src/Tooltip.vue

@@ -71,9 +71,9 @@ export default {
 </script>

 <style>
-    .v-popper--theme-tooltip .v-popper__inner {
+    .v-popper-theme-tooltip .v-popper-inner {


This seems to be having unexpected effects - the styling doesn't apply to the tooltip anymore.
You can try modifying the font size to something huge and then removing the one of the dashes, you realise the font change doesn't apply.
Can we keep the old version? Will it automatically change every time the linter is run?

The namings here violated the rule selector-class-pattern.
To revert it back to the old version and prevent stylelint from throwing an error, I have added a line comment on top of it to disable stylelint for this part. (this is how stylelint is disabled for a particular part of code)

yucheng11122017

LGTM! Thanks for the work @LamJiuFong :)

Jiu Fong Lam added 2 commits February 15, 2024 23:43

Migrate stylelint to latest version ^16.0.0

65fa619

Add dependency "stylelint-config-recommended-vue" Fix syntax to obey new rules

Fix minor bug in web3Form.ts, update class name

d154e83

Jiu Fong Lam added 3 commits February 25, 2024 15:18

Merge branch 'master' into 2292-migrate-stylelint

1ebd08e

Update test

e4939a9

Update test

3af96b2

Add plugin @stylistic/stylelint-plugin and postcss@^8.4.35

1967a6a

Merge branch 'master' into 2292-migrate-stylelint

02aca7f

kaixin-hc reviewed Feb 25, 2024

View reviewed changes

kaixin-hc approved these changes Feb 25, 2024

View reviewed changes

kaixin-hc changed the title ~~[Request for Early Review] Migrate stylelint to latest version #2292~~ Migrate stylelint to latest version #2292 Feb 25, 2024

Jiu Fong Lam added 2 commits February 25, 2024 22:19

Remove unnecessary comment

76bade5

Remove unnecessary comments

32cdfde

Merge branch 'master' into 2292-migrate-stylelint

2cd003b

yucheng11122017 reviewed Feb 26, 2024

View reviewed changes

Fix bug on tooltip

a33e489

yucheng11122017 approved these changes Feb 28, 2024

View reviewed changes

Merge branch 'master' into 2292-migrate-stylelint

f9ba940

kaixin-hc merged commit 9336559 into MarkBind:master Feb 29, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate stylelint to latest version #2292 #2425

Migrate stylelint to latest version #2292 #2425

LamJiuFong commented Feb 15, 2024 •

edited

Loading

LamJiuFong commented Feb 15, 2024

EltonGohJH commented Feb 16, 2024

codecov bot commented Feb 25, 2024 •

edited

Loading

LamJiuFong commented Feb 25, 2024

kaixin-hc Feb 25, 2024

kaixin-hc Feb 25, 2024

LamJiuFong Feb 25, 2024

kaixin-hc Feb 25, 2024

kaixin-hc Feb 25, 2024

LamJiuFong Feb 25, 2024

kaixin-hc left a comment

kaixin-hc commented Feb 25, 2024

LamJiuFong commented Feb 26, 2024

yucheng11122017 left a comment

yucheng11122017 Feb 26, 2024

LamJiuFong Feb 28, 2024

yucheng11122017 left a comment

Migrate stylelint to latest version #2292 #2425

Migrate stylelint to latest version #2292 #2425

Conversation

LamJiuFong commented Feb 15, 2024 • edited Loading

LamJiuFong commented Feb 15, 2024

EltonGohJH commented Feb 16, 2024

codecov bot commented Feb 25, 2024 • edited Loading

Codecov Report

LamJiuFong commented Feb 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaixin-hc left a comment

Choose a reason for hiding this comment

kaixin-hc commented Feb 25, 2024

LamJiuFong commented Feb 26, 2024

yucheng11122017 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

LamJiuFong commented Feb 15, 2024 •

edited

Loading

codecov bot commented Feb 25, 2024 •

edited

Loading