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

Replace PDFs with SVGs from desktop/.../assets where available #7761

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

Conversation

acb-mv
Copy link
Contributor

@acb-mv acb-mv commented Mar 5, 2025

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:
image

and the account screen:
image


This change is Reviewable

@acb-mv acb-mv added the iOS Issues related to iOS label Mar 5, 2025
@acb-mv acb-mv self-assigned this Mar 5, 2025
Copy link

linear bot commented Mar 5, 2025

Copy link
Contributor

@rablador rablador left a 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

@pinkisemils
Copy link
Collaborator

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.

Copy link
Contributor

@rablador rablador left a 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

Copy link
Contributor Author

@acb-mv acb-mv left a 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

@acb-mv
Copy link
Contributor Author

acb-mv commented Mar 6, 2025

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 UIImage(named:"...") with an extension on UIImage that produces a UIImage.closeSmall whose dimensions are suitable for our purposes, but doesn't burden the caller with caring about where it gets the image from.

Copy link
Contributor

@rablador rablador left a 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:
Simulator Screenshot - iPhone 16 Pro - 2025-03-06 at 16.33.53.png
Simulator Screenshot - iPhone 16 Pro - 2025-03-06 at 16.32.57.png
Simulator Screenshot - iPhone 16 Pro - 2025-03-06 at 16.32.28.png
Simulator Screenshot - iPhone 16 Pro - 2025-03-06 at 16.28.17.png
Simulator Screenshot - iPhone 16 Pro - 2025-03-06 at 16.25.24.png
Simulator Screenshot - iPhone 16 Pro - 2025-03-06 at 16.24.50.png

Reviewable status: 0 of 73 files reviewed, all discussions resolved

@acb-mv
Copy link
Contributor Author

acb-mv commented Mar 6, 2025

A lot of those are part of the close icon regression. I'm in the process of refactoring how we retrieve asset UIImages, which will fix this.

@acb-mv
Copy link
Contributor Author

acb-mv commented Mar 6, 2025

Our image assets are now loaded and sized as appropriate in UIImage+Assets.swift, which fixes most of these.
The checkmark in the filter edit dialog is still a bit more slender than the PDF one, though.

Copy link
Contributor

@rablador rablador left a 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: :shipit: complete! all files reviewed, all discussions resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants