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

[Paywalls V2] Icon Component #4655

Merged
merged 7 commits into from
Jan 14, 2025
Merged

[Paywalls V2] Icon Component #4655

merged 7 commits into from
Jan 14, 2025

Conversation

joshdholtz
Copy link
Member

@joshdholtz joshdholtz commented Jan 13, 2025

Motivation

New icon component

Description

Screenshot 2025-01-13 at 3 33 13 PM

Copy link

emerge-tools bot commented Jan 13, 2025

1 build increased size

Name Version Download Change Install Change Approval
Paywalls
com.revenuecat.PaywallsTester
1.0 (1) 11.7 MB ⬆️ 198.7 kB (1.73%) 43.1 MB ⬆️ 640.7 kB (1.52%) N/A

Paywalls 1.0 (1)
com.revenuecat.PaywallsTester

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 640.7 kB (1.52%)
Total download size change: ⬆️ 198.7 kB (1.73%)

Largest size changes

Item Install Size Change
DYLD.String Table ⬆️ 116.4 kB
📝 RevenueCatUI.IconComponentViewModel.IconComponentViewModel ⬆️ 83.7 kB
Code Signature ⬆️ 16.3 kB
DYLD.Exports ⬆️ 6.0 kB
PaywallsTester.debug.dylib ⬆️ 768 B
View Treemap

Image of diff


🛸 Powered by Emerge Tools

Comment trigger: Size diff threshold of 100.00kB exceeded

@joshdholtz joshdholtz force-pushed the paywalls-v2/icon-component branch from 37b7a01 to 9dcd0d6 Compare January 13, 2025 22:43
@joshdholtz joshdholtz force-pushed the paywalls-v2/icon-component branch from ffb832a to b0bef46 Compare January 14, 2025 03:31
@joshdholtz joshdholtz requested review from a team January 14, 2025 03:40
@joshdholtz joshdholtz marked this pull request as ready for review January 14, 2025 03:50
Copy link
Contributor

@tonidero tonidero 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 just wondering if we could unify IconComponent and ImageComponent... There are some differences so not totally sure TBH... But LGTM!

public let svg: String
public let png: String
public let heic: String
public let webp: String
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm do we need all 4 formats? Could we just keep one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah 😅 I totally only need heic here! Will remove the rest 👋

public let padding: Padding?
public let margin: Padding?
public let color: ColorScheme
public let iconBackground: IconBackground?
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so the main differences with an ImageComponent are going to be the background + the iconName? (not sure if I missed any) I wonder if we should just reuse ImageComponent...

Copy link
Member

Choose a reason for hiding this comment

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

And if not for ImageComponent, maybe we can still get away with using ImageComponentView for icons?

Copy link
Member Author

@joshdholtz joshdholtz Jan 14, 2025

Choose a reason for hiding this comment

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

And no overlayColor and also no maskShape 🤷‍♂️

Could maybe combine them later but I didn't want to think that hard and risk breaking things in the state them I'm in right now 😛

@joshdholtz joshdholtz merged commit d86e100 into main Jan 14, 2025
10 checks passed
@joshdholtz joshdholtz deleted the paywalls-v2/icon-component branch January 14, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants