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

Linting SVG files #1642

Merged
merged 22 commits into from
Nov 6, 2023
Merged

Linting SVG files #1642

merged 22 commits into from
Nov 6, 2023

Conversation

ericfennis
Copy link
Member

What is the purpose of this pull request?

  • New Icon
  • Bug fix
  • New Feature
  • Documentation update

Description

This adds a feature to lint SVG files. I've implemented the @html-eslint with some rules that match our way of writing the SVGs.

Because it is part of ESLint, lint-staged will check the format of SVGs.
I also added a GitHub action to check this when a PR is opened. GitHub will automatically add a comment if the formatting is incorrect.

@github-actions github-actions bot added 📦 dependencies Pull requests that update a dependency file 🎨 icon About new icons 💎 vue package Lucide Vue package 🫧 metadata Improved metadata 🅰️ angular package About the Angular package labels Oct 27, 2023
@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Added or changed icons

icons/theater.svg

Preview cohesion icons/scissors-square.svg
icons/theater.svg
icons/repeat-2.svg
Preview stroke widths icons/theater.svg
icons/theater.svg
icons/theater.svg
DPI Preview (24px) icons/theater.svg
Icon X-rays icons/theater.svg

Copy link
Member

@jguddas jguddas left a comment

Choose a reason for hiding this comment

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

Maybe split the PR into:

  • Adding linting of svgs
  • Run linting of svgs in PRs

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.github/workflows/pull-request.yml Outdated Show resolved Hide resolved
.github/workflows/pull-request.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the 🫧 metadata Improved metadata label Nov 1, 2023
@ericfennis
Copy link
Member Author

@jguddas Thanks for your feedback!

@ericfennis ericfennis requested a review from jguddas November 1, 2023 20:31
@ericfennis ericfennis marked this pull request as ready for review November 1, 2023 20:31
@@ -29,21 +29,22 @@
"generate:contributors": "node ./scripts/updateContributors.mjs icons/*.svg",
"generate:nextJSAliases": "node ./scripts/generateNextJSAliases.mjs",
"postinstall": "husky install",
"lint:js": "eslint --ext .ts,.js,.mjs ./{packages/lucide,scripts}",
Copy link
Member

Choose a reason for hiding this comment

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

Now the :js name does not make sense anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

true, i've renamed it to lint:es 😛

@@ -0,0 +1,20 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have this file now?

Copy link
Member Author

Choose a reason for hiding this comment

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

When running Eslint running from the root, it reads also TS files, which requires a tsconfig in the root.

@github-actions github-actions bot added the 🫧 metadata Improved metadata label Nov 6, 2023
@ericfennis ericfennis merged commit 288edde into main Nov 6, 2023
12 of 14 checks passed
@ericfennis ericfennis deleted the svg-eslint branch November 6, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🅰️ angular package About the Angular package 📦 dependencies Pull requests that update a dependency file 🫧 metadata Improved metadata 💎 vue package Lucide Vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants