Skip to content

fix: auto icons override default icons #1616

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

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

Conversation

omerfardemir
Copy link

just removed return statement in line 35.

return wxt.logger.warn(

Manual Testing

  1. have default icons in the public folder public/icon/*
  2. install auto icons
  3. add a icon to <src>/assets/icon.png
  4. run npm run dev

This PR closes #1592

@omerfardemir omerfardemir requested a review from Timeraa as a code owner April 25, 2025 23:29
Copy link

netlify bot commented Apr 25, 2025

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit 2dabfec
🔍 Latest deploy log https://app.netlify.com/projects/creative-fairy-df92c4/deploys/683c1a86fdf06100085aa134
😎 Deploy Preview https://deploy-preview-1616--creative-fairy-df92c4.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 project configuration.

@Timeraa
Copy link
Member

Timeraa commented May 29, 2025

Could you add a test for this?

@omerfardemir omerfardemir requested a review from aklinker1 as a code owner May 31, 2025 21:42
@omerfardemir
Copy link
Author

I added tests and here is output:

✓ src/__test__/index.test.ts (9 tests) 23ms
   ✓ auto-icons module > options handling > should use default options when not provided 5ms
   ✓ auto-icons module > options handling > should merge custom options with defaults 1ms
   ✓ auto-icons module > error handling > should warn when disabled 4ms
   ✓ auto-icons module > error handling > should warn when base icon not found 1ms
   ✓ auto-icons module > manifest generation > should update manifest with icons 2ms
   ✓ auto-icons module > manifest generation > should warn when overwriting existing icons in manifest 1ms
   ✓ auto-icons module > icon generation > should generate icons with correct sizes 2ms
   ✓ auto-icons module > icon generation > should apply grayscale in development mode but not in production 1ms
   ✓ auto-icons module > public paths > should add icon paths to public paths 1ms

 Test Files  1 passed (1)
      Tests  9 passed (9)
   Start at  00:40:30
   Duration  630ms (transform 35ms, setup 0ms, collect 84ms, tests 23ms, environment 0ms, prepare 185ms)

@Timeraa
Copy link
Member

Timeraa commented Jun 1, 2025

Thank you, did I not add any tests to that module when I made it? Oopsies haha.

Looks all fine from me so @aklinker1 can merge when he approves of it

Copy link
Collaborator

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

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

I'm alive! Been busy with some other life stuff. Should be back on GitHub and discord soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests don't import or test the actual module's logic, it just copies some code into the test file.

I can help write tests that import and use the actual module... Sometime soon 🤞

Copy link
Author

@omerfardemir omerfardemir Jun 1, 2025

Choose a reason for hiding this comment

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

@aklinker1 Actually, I checked all the modules but I couldn't find an example that imports and tests a module. I also checked the Unit Test documention but I couldn't find the answer I was looking for. If you give me a clue, I'd be happy to improve it. Thanks for your time 😊

@Timeraa
Copy link
Member

Timeraa commented Jun 1, 2025

I'm alive! Been busy with some other life stuff. Should be back on GitHub and discord soon.

No worries everyone gets through things like that in life sometimes! I'm currently going through a lot of it as well

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.

Auto icons doesnt override default icons
3 participants