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

feat(mobile): new icons #14963

Merged
merged 3 commits into from
Oct 22, 2024
Merged

feat(mobile): new icons #14963

merged 3 commits into from
Oct 22, 2024

Conversation

Nodonisko
Copy link
Contributor

@Nodonisko Nodonisko commented Oct 20, 2024

I recommend check this commit by commit, first one is creating new mobile icons package and some refactoring. Second one is just replacing deprecated package with new one.

Description

This new icons package is created with main focus on performance and minimal performance overhead so it should be super fast. I also did some refactoring and simplification of some Button components which also helped with performance little bit.

Related Issue

Resolve #12433
Fixes #11596
Fixes #14311

Screenshots:

@Nodonisko Nodonisko force-pushed the feat/new-icons-mobile branch from 9cfbaaa to 617a7a3 Compare October 20, 2024 18:03
Copy link

socket-security bot commented Oct 20, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/[email protected]

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@Nodonisko Nodonisko force-pushed the feat/new-icons-mobile branch from 617a7a3 to ac8ca0f Compare October 20, 2024 18:18
@Nodonisko Nodonisko marked this pull request as ready for review October 20, 2024 18:19
@Nodonisko Nodonisko requested review from a team, jvaclavik and adamhavel as code owners October 20, 2024 18:19
@Nodonisko Nodonisko changed the title chore: all icons in new package feat(mobile): new icons Oct 20, 2024
@Nodonisko
Copy link
Contributor Author

@SocketSecurity ignore npm/[email protected]

@Nodonisko
Copy link
Contributor Author

ttf2woff2 package looks safe and it's used only locally for generating font files so it should be safe

@Nodonisko Nodonisko force-pushed the feat/new-icons-mobile branch 2 times, most recently from e40ec9f to e2886e2 Compare October 20, 2024 18:29
@matejkriz matejkriz added the mobile Suite Lite issues and PRs label Oct 21, 2024
color?: IconColor;
};

export const Icon = ({ name, size = 'large', color = 'iconDefault' }: IconProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what exact change caused Device info and Open passphrase buttons to be cut off, but looks ok on develop.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not able to reproduce this one, can you please try again? Maybe it was some random temp issue.

@matejkriz
Copy link
Member

I guess this can fix also
#14311
and
#11596
right, @Nodonisko ?

@Nodonisko
Copy link
Contributor Author

Yes it should fix all icon related issue we have now.

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

I didn't go through the individual icon changes, I mostly check only the first commit and it works nice, including switching between dark and light mode 🎉

Only issue I found out is the cut off buttons in the Device switcher.

It would be great to update README of @suite-common/icons too, but it can be a followup.

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

It looks like the icon for Safe 3 is broken only in the font generated from correct SVG, so maybe there are more icons broken, only not so obvious?

@Nodonisko Nodonisko force-pushed the feat/new-icons-mobile branch 2 times, most recently from b7718a7 to 9820e56 Compare October 22, 2024 12:37
@Nodonisko Nodonisko force-pushed the feat/new-icons-mobile branch from 9820e56 to 5b31801 Compare October 22, 2024 12:54
Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

👏

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

👏

@Nodonisko Nodonisko merged commit b3924ee into develop Oct 22, 2024
87 of 88 checks passed
@Nodonisko Nodonisko deleted the feat/new-icons-mobile branch October 22, 2024 14:18
komret pushed a commit that referenced this pull request Nov 14, 2024
* feat(mobile): new mobile icons package + icon font

* chore: replace deprecated icons in mobile app

* fix: fix broken icons and add README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Suite Lite issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sporadic missing icons in dark mode Iconset replacement Icons don't follow theme right after theme change
3 participants