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

prune: remove unused icons #429

Merged
merged 1 commit into from
Jan 4, 2023
Merged

Conversation

MatissJanis
Copy link
Member

Removing unused icons and adding eslint-ignore rule to the remaining.

@netlify
Copy link

netlify bot commented Jan 3, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 4790ff2
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/63b4a324fdf6fb0008102d37
😎 Deploy Preview https://deploy-preview-429--actualbudget.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.

@MatissJanis MatissJanis marked this pull request as ready for review January 3, 2023 21:54
@j-f1
Copy link
Contributor

j-f1 commented Jan 3, 2023

IMO we should keep the icons since they may be useful in the future — maybe add an ignore to the entire folder for now?

@jlongster
Copy link
Contributor

This looks great! svgs definitely went through some evolution and need some cleanup. Some context: the svgs in the root svg folder are very old (and can be deleted if not used). The svgs in v1 are an assortment from various places. A few of them are still used throughout the app. The svgs in v2 are all icons from https://www.streamlinehq.com (actually, a subset of them I believe). I'd keep around icons in v2 even if we don't use them because they are meant to be a readily available set if icons, so you can import them and use them (even if we aren't using them yet). That was my plan at least.

The generate script is also old and generate-new is the one that is used to convert svgs to React components.

And I agree with @j-f1: was gonna suggest just ignoring the whole folder. These are generated files that don't need to be linted.

But this cleanup is AWESOME. definitely a messy area!

@MatissJanis
Copy link
Member Author

This PR doesn't touch v1 or v2 icons btw. Only the legacy ones that are unused.

A PR approval ✅ would be appreciated if you think this is good-to-go.

@jlongster
Copy link
Contributor

Yeah, this PR looks good for that reason :) just giving some context when I see stuff! I'm in a rush right now so haven't actually looked to closely to r+ but it should be fine

Copy link
Contributor

@jlongster jlongster 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, thanks for the cleanup!

@MatissJanis MatissJanis merged commit 1630654 into master Jan 4, 2023
@MatissJanis MatissJanis deleted the matiss/unused-variables-3 branch January 4, 2023 17:33
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
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.

None yet

3 participants