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] Add purchase button activity indicator #4787

Merged
merged 7 commits into from
Feb 14, 2025

Conversation

joshdholtz
Copy link
Member

Motivation

Need a way to show when there is activity happening with a purchase button

Description

Screen.Recording.2025-02-13.at.8.11.14.PM.mov

@joshdholtz joshdholtz changed the base branch from main to paywalls-v2/visible-property February 14, 2025 02:44
@joshdholtz joshdholtz requested review from a team February 14, 2025 02:44
Copy link
Member

@ajpallares ajpallares left a comment

Choose a reason for hiding this comment

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

Good! Just a couple of small comments

.progressViewStyle(CircularProgressViewStyle(tint: bestTint(for: colorInfo)))
case .image, .none:
ProgressView()
.progressViewStyle(CircularProgressViewStyle(tint: .white))
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 if the tint here should depend on the colorScheme and/or on something else. You know, to prevent showing the while progress view on a white background. But then, I don't even know if this case is even possible (looking at the Dashboard, it seems like Buttons only support color-based backgrounds)

Copy link
Member

Choose a reason for hiding this comment

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

Yea I think the best version would be to base the tint on the image pixels underneath the circular progress view. But that can also come in a next iteration imo.

Copy link
Member

Choose a reason for hiding this comment

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

Or actually, @joshdholtz are there any plans on making the loading indicator have a configurable color on the dashboard? That would avoid the need for us to be smart about it.

Copy link
Member

Choose a reason for hiding this comment

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

If not, another way of trying to be smart about it could be to try and use the same color as the (first?) text component in the button. But I'm sure there are edge cases for this (e.g. no text or multiple text components). Just putting ideas out there

Copy link
Member Author

@joshdholtz joshdholtz Feb 14, 2025

Choose a reason for hiding this comment

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

are there any plans on making the loading indicator have a configurable color

@JayShortway I didn't want to because its not something our users should have to think about but I'm not opposed if we make it an optional style at some point

Not sure if the tint here should depend on the colorScheme and/or on something else

@ajpallares The calculated tint does take the color scheme into effect so we should be good!

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I added a blur to the background it so it should make it even easier for either tint color to be visible 🤞

Comment on lines +48 to +56
let gradient = colorInfo.toGradient()
let averageBrightness = gradient.stops
.compactMap { $0.color.brightness() }
.reduce(0, +) / CGFloat(gradient.stops.count)
return averageBrightness > 0.6 ? .black : .white
Copy link
Member

Choose a reason for hiding this comment

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

I think this is ok as a first version and will work 99%, but perhaps we could try and be smarter about this, in the sense of giving the colors in the center a higher weight when computing the averageBrightness than to the colors near the edges (because the ProgressView will appear in the center). Although to get a counterexample of this it probably means chosing a bad gradient combination 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love to be smarter about this in the future 😅 It's not perfect but its something 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Totally! :shipit: 🚀

Copy link
Member

@JayShortway JayShortway left a comment

Choose a reason for hiding this comment

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

Fancy!

extension Color {
func brightness() -> CGFloat {
guard let uiColor = UIColor(self).cgColor.components else { return 1.0 }
return (uiColor[0] * 299 + uiColor[1] * 587 + uiColor[2] * 114) / 1000
Copy link
Member

Choose a reason for hiding this comment

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

What are these magic numbers? Maybe a comment or well-named constants could explain them?

.progressViewStyle(CircularProgressViewStyle(tint: bestTint(for: colorInfo)))
case .image, .none:
ProgressView()
.progressViewStyle(CircularProgressViewStyle(tint: .white))
Copy link
Member

Choose a reason for hiding this comment

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

Yea I think the best version would be to base the tint on the image pixels underneath the circular progress view. But that can also come in a next iteration imo.

Base automatically changed from paywalls-v2/visible-property to main February 14, 2025 12:57
@joshdholtz joshdholtz force-pushed the paywalls-v2/purchase-button-activity-indicator branch from 3759244 to ac405b7 Compare February 14, 2025 14:58
@joshdholtz joshdholtz force-pushed the paywalls-v2/purchase-button-activity-indicator branch from b1e9e19 to 7770f2c Compare February 14, 2025 17:14
@joshdholtz joshdholtz marked this pull request as ready for review February 14, 2025 17:48
@joshdholtz joshdholtz merged commit 6331687 into main Feb 14, 2025
9 of 10 checks passed
@joshdholtz joshdholtz deleted the paywalls-v2/purchase-button-activity-indicator branch February 14, 2025 17:51
This was referenced Feb 19, 2025
@MortenGregersen
Copy link

Love it! 🎉

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.

4 participants