Skip to content

Stylelint install and config#1427

Merged
LlGC-jop merged 21 commits intoUniversalViewer:devfrom
LlGC-jop:stylelint
Jun 11, 2025
Merged

Stylelint install and config#1427
LlGC-jop merged 21 commits intoUniversalViewer:devfrom
LlGC-jop:stylelint

Conversation

@LlGC-jop
Copy link
Copy Markdown
Contributor

Adds stylelint package plus less support, as well as config that will overlook the major issues in the project for now. Everything else should be auto-fixable as a future PR.

Also adds VSCode settings for the project so .less files can have issues highlighted (but won't interfere with saving/committing)

@vercel
Copy link
Copy Markdown

vercel bot commented May 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2025 9:08am

Copy link
Copy Markdown
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

As with #1426: do we need to integrate this into package.json scripts and/or GitHub actions, or is it already going to happen "magically" somehow?

@LlGC-jop
Copy link
Copy Markdown
Contributor Author

All conflicts should have been resolved.

The stylelint rules file has quite a few rules disabled because they would take quite a bit of work to fix, and should be manually checked.

As with #1426: do we need to integrate this into package.json scripts and/or GitHub actions, or is it already going to happen "magically" somehow?

I've added an npm command to run stylelint, however I haven't added it to the git workflow file because:

a) It makes a lot of changes
b) Some of those changes I'm not sure about, so would need sense-checking.

Once this is merged with dev I'll create a new branch and run an auto-fix, then check the changes to make sure nothing breaks.

If that's all fine then I think we can add stylelint to the workflow.

Then as a longer term job, we can look at turning the disabled rules on one-by-one and fixing things where appropriate.

@LlGC-jop LlGC-jop marked this pull request as ready for review May 20, 2025 11:28
Copy link
Copy Markdown
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @LlGC-jop! A few thoughts:

1.) Maybe the best way to structure the package.json scripts would be to rename the current lint to lint-code and then redefine it as npm run lint-code lint-styles. Then we have a universal lint command, plus the ability to lint things separately.

2.) I realize that following my proposal above would cause changes to get applied, which you might not be ready to move forward with. But my argument is that we shouldn't commit tool configurations that will change code without also committing the code changes. I think it makes sense to do it all together, since if the changes cause problems, we'll have to change the configuration -- so we might as well start with a working configuration, instead of an unproven one. I don't mind if that takes a little extra time to get this PR done.

3.) Semi-related note: I think you wanted to adjust the prettier configuration to look at less files; would it make sense to do that in a separate PR and commit any related changes first? Is it possible that doing that will reduce the number of changes introduced by stylelint?

We can discuss further at today's stand-up!

@LlGC-jop
Copy link
Copy Markdown
Contributor Author

@demiankatz I've had a look over the CSS changes and am largely happy that they're fine - though I won't claim that I haven't missed anything :)

I've requested a review from @LanieOkorodudu, if that's ok with you Lanie, just to have another pair of eyes look over this build in case I've missed anything.

The few instances I mentioned yesterday where properties were repeated due to -moz and -webkit prefixes being removed were all in the mixins file, most of which aren't being used.

So I'm happy to leave these duplicates as they shouldn't affect anything - anyone writing new CSS shouldn't be using prefixes any more afaik and therefore won't need the mixins.

@LlGC-jop LlGC-jop marked this pull request as ready for review May 22, 2025 08:24
@LanieOkorodudu
Copy link
Copy Markdown
Collaborator

@demiankatz I've had a look over the CSS changes and am largely happy that they're fine - though I won't claim that I haven't missed anything :)

I've requested a review from @LanieOkorodudu, if that's ok with you Lanie, just to have another pair of eyes look over this build in case I've missed anything.

The few instances I mentioned yesterday where properties were repeated due to -moz and -webkit prefixes being removed were all in the mixins file, most of which aren't being used.

So I'm happy to leave these duplicates as they shouldn't affect anything - anyone writing new CSS shouldn't be using prefixes any more afaik and therefore won't need the mixins.

@LlGC-jop I am afraid, I will not be able to really check this today as I have the whole day course to attend. Will be it okay to request @jamesmisson, if he can spare some time to check. Otherwise I will do it tomorrow. Thanks.

Copy link
Copy Markdown
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @LlGC-jop -- I gave these files a fairly careful review and spotted a handful of things that might be problems (or might just point to my ignorance about details of LESS). :-)

Comment thread package.json
Comment thread src/content-handlers/iiif/extensions/uv-aleph-extension/lib/aleph.css Outdated
Comment thread src/content-handlers/iiif/extensions/uv-aleph-extension/lib/aleph.css Outdated
Comment thread src/content-handlers/iiif/extensions/uv-aleph-extension/lib/aleph.css Outdated
Comment thread src/content-handlers/iiif/modules/uv-shared-module/css/mixins.less
Comment thread src/content-handlers/iiif/modules/uv-shared-module/css/mixins.less Outdated
@LanieOkorodudu
Copy link
Copy Markdown
Collaborator

LanieOkorodudu commented May 23, 2025

@LlGC-jop, I had a look through the changes and also read Demian’s suggestions, based on his detailed feedback and familiarity with the context, I feel he’s probably in the best position to give the most useful review on this one.
I hope you don’t mind me passing this on to you, @demiankatz

Also, I just wanted to mention that my expertise in this area isn’t as strong as Demian, so I didn’t feel confident enough to add a formal review or approval. I really appreciate being included, though, and I hope it’s okay that I defer to the expert here. Thanks again!

@demiankatz
Copy link
Copy Markdown
Contributor

No problem, @LanieOkorodudu -- let's wait and see what @LlGC-jop decides to do. Once we reach a final revision, if you have time, it might be helpful to do some side-by-side comparisons of the build here against the current dev build. In theory, there should be no visible side effects of reformatting the LESS code, but it may be worth doing some quick comparisons to be absolutely sure nothing has unexpectedly changed.

@LlGC-jop
Copy link
Copy Markdown
Contributor Author

@demiankatz @LanieOkorodudu

Hi both, have added comments/made changes, but in short:

  • I've created lint and fix commands to run combos
  • I've made a few tweaks to get stylelint to ignore the case of those font/legibility properties.
  • :: is correct and should work fine
  • I've removed duplicate properties and that old comment

I've also merged the latest dev into my branch and regenerated the package lock file, so this should be up-to-date with the recent merge into dev of 4.2.0 that James did yesterday.

Copy link
Copy Markdown
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @LlGC-jop, this all looks good to me now! I'll leave this open in case anyone else wants to test/review before merging. We can discuss at today's standup.

@LanieOkorodudu
Copy link
Copy Markdown
Collaborator

I’ve reviewed all the file changes and compared the additions, fixes, and updates, everything looks good to me. I’ll leave the merging to @demiankatz, as he’s happy with all the changes.

Thank you, @LlGC-jop, for the massive piece of work you’ve done here, and thanks to @demiankatz for your valuable feedback.

@LlGC-jop LlGC-jop merged commit f252551 into UniversalViewer:dev Jun 11, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants