-
Notifications
You must be signed in to change notification settings - Fork 381
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
Replace PDFs with SVGs from desktop/.../assets
where available
#7761
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some icons need adjustment. I think we should fix that in this PR.
Reviewable status: 0 of 72 files reviewed, all discussions resolved
Specifically, the arrow for the login text field should be adjusted so that the whole text field is not blown out of proportion, and the cog and account icons maybe should be increas in size? @waahlnaden probably has some ideas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 72 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have adjusted HeaderBarView
to fudge the sizes of the button images to compensate for SVGs not being trimmed.
Reviewable status: 0 of 73 files reviewed, all discussions resolved
I'm looking at the login screen now, and it looks like the close image (which has a larger intrinsic size) is blowing it out, not the arrow. The desktop assets had only one close image size, whereas we had the same image with different dimensions. I think a solution may be to replace our uses of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header icons look good, but we should fix a couple more:
Reviewable status: 0 of 73 files reviewed, all discussions resolved
A lot of those are part of the close icon regression. I'm in the process of refactoring how we retrieve asset |
Our image assets are now loaded and sized as appropriate in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot better now, checkmarks included. However, the close buttons are too small now, and the info buttons too large. What do @waahlnaden think?
Reviewed 70 of 72 files at r1, 27 of 27 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
I inspected the info icon, and the original was 18x18 uncropped; I adjusted the new one to match that. |
The unit tests fail to build in the CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example from the login screen:
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have adjusted the close button (the actual image was, as with other uncropped images, taking up only 5/6 of its frame).
Reviewable status: 96 of 97 files reviewed, all discussions resolved (waiting on @rablador)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit tests are fixed as well (at the cost of getting rid of elegant UIImage(resource: .iconFoo)
initialisers and making do with UIImage(named: "IconFoo")!
instead, to allow the extension to be added to the tests target without dragging in the entire asset catalogue).
Reviewable status: 95 of 97 files reviewed, all discussions resolved (waiting on @rablador)
This replaces PDF images with SVGs where the desktop assets had suitable ones. A few things are unreplaced (mostly button shapes). The elements may appear smaller on screen due to the SVGs not being cropped as the PNGs were (the PNGs seem to have been handcrafted for the app and not mechanically derived from the SVGs).
This is what the main screen looks like:

and the account screen:

This change is