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

Web 2936 resolve stylelint issues in v16 upgrade #507

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nickbergquist
Copy link
Contributor

https://vandam.atlassian.net/browse/WEB-2938

Jira ticket was aimed at addressing new linting errors arising from the Stylint upgrade to v.16 in vam-web. I have updated to the same version (including the standard config.) here in vam-fractal for consistency.

@nickbergquist nickbergquist added the dependencies Pull requests that update a dependency file label Apr 9, 2024
@nickbergquist nickbergquist self-assigned this Apr 9, 2024
position: absolute;
top: 0;
width: 90%;

@media (min-width: 1000px) {
@media (width >= 1000px) {
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 this syntax is much clearer!
but why not make the change everywhere?

Copy link
Contributor Author

@nickbergquist nickbergquist Apr 10, 2024

Choose a reason for hiding this comment

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

@methodog Well, interestingly this highlights an issue with the linting in that errors are reported when the media feature range is in the following format

@media (min-width: 1000px) {...}

but if we are using calculations and/or variables then this is not reported in Stylelint v.15:

@media (min-width: map-get($nav-breakpoints, $breakpoint)) {...}

So it seems we should keep in mind when addressing linting errors and indeed developing from new that there may be other somewhat hidden occurrences. If a search across the vam-fractal project is made using @media (min- then other instances can be found in the following files:

src/components/blocks/site-nav/_site-nav.scss
src/components/mixins/breakpoints/readme.md
src/components/mixins/breakpoints/_breakpoints.scss

Furthermore, a search for @media (max- reveals further instances not detected by the linter in:

src/components/blocks/site-nav/_site-nav.scss
src/components/mixins/breakpoints/_breakpoints.scss

Also of note is that the changes demanded by the linter are abstractions; @media (width >= 1000px) {...} is compiled to @media (min-width: 1000px) {...} in the CSS.

Copy link
Contributor

@methodog methodog Apr 22, 2024

Choose a reason for hiding this comment

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

thanks @nickbergquist
interesting point about the syntax change being entirely a SASS abstraction - hopefully CSS will catch up with the improved syntax!
back on the point about switching to the new syntax consistently throughout, presumably we are still free to do that, even if not forced to by the linter?

Copy link
Contributor

@methodog methodog Apr 22, 2024

Choose a reason for hiding this comment

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

ah - it looks as though you did just that, with a commit between my comments!

@nickbergquist nickbergquist force-pushed the web-2936-resolve-stylelint-issues-in-v16-upgrade branch from 4efec5f to 1d1992a Compare April 10, 2024 14:15
@nickbergquist
Copy link
Contributor Author

I have added an exclusion in the root .stylelintrc.json config. to override the "media-feature-name-unknown" rule. This is because using SASS calculations and/or variables in the form of say:

@media (width <= map.get(mixins.$breakpoints-breakpoints, "small")) {...}

is not registered as a problem using Stylelint v.15 but is using the latest version. Whilst this is not evident in vam-fractal as yet it is in vam-web and I have included the exclusion here for standardisation between the configuration files of both projects.

@methodog
Copy link
Contributor

Thanks @nickbergquist , I think this was the best way to proceed all round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants