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 components/file split #4200

Merged

Conversation

jamesrb1
Copy link
Contributor

@jamesrb1 jamesrb1 commented Aug 20, 2024

Removes code we won't be using for some time and splits up some large files into component files.

@jamesrb1
Copy link
Contributor Author

Left in the Tiers Toggle and Tiers Selector because they are used by Tiers. This may be revisited in the future.

@jamesrb1 jamesrb1 marked this pull request as ready for review August 20, 2024 21:08
@jamesrb1 jamesrb1 requested a review from joshdholtz August 20, 2024 21:08
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This is a much better starting point than my monofile I had going 😅

A few suggestions to delete things we don't need but looks good to me since this will all be behind a flag and we'll iterate quickly 👍

@@ -0,0 +1,198 @@
//
// File.swift
Copy link
Member

Choose a reason for hiding this comment

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

This is how you can tell how much of a POC this was 😛

Comment on lines +65 to +71
func getLocalization(_ locale: Locale, _ displayString: DisplayString) -> String {
if let found = displayString.value[locale.identifier] {
return found
}

return displayString.value.values.first!
}
Copy link
Member

Choose a reason for hiding this comment

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

Note for later: this will get replaced with proper localization lookup... this was very proof of concept

Comment on lines +89 to +114
var landscapeLeftComponents: [PaywallComponent] {
return self.components.filter { component in
guard let displayPreferences = component.displayPreferences else {
return true
}
return displayPreferences.contains(.landscapeLeft)
}
}

var landscapeRightComponents: [PaywallComponent] {
return self.components.filter { component in
guard let displayPreferences = component.displayPreferences else {
return true
}
return displayPreferences.contains(.landscapeRight)
}
}

var portraitComponents: [PaywallComponent] {
return self.components.filter { component in
guard let displayPreferences = component.displayPreferences else {
return true
}
return displayPreferences.contains(.portrait)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Note for later: this was done mainly for proof of conept. We will be having more discussions about how to properly handle landscape

@@ -98,16 +79,6 @@ public enum PaywallComponent: Decodable, Sendable, Hashable, Equatable {
return component.displayPreferences
case .image(let component):
return component.displayPreferences
case .video(let component):
Copy link
Member

Choose a reason for hiding this comment

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

This whole displayPreferences could honestly probably get deleted. This was for the portrait/landscape POC that we will probably end up rethinking Its just a bit messy ATM so I'm find getting rid of this 👍

@@ -125,16 +96,6 @@ public enum PaywallComponent: Decodable, Sendable, Hashable, Equatable {
return component.focusIdentifiers
case .image(let component):
return component.focusIdentifiers
case .video(let component):
return component.focusIdentifiers
Copy link
Member

Choose a reason for hiding this comment

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

Same with this! This was for game controller POC so this whole focusIdentifier thing can get deleted 👋

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I was thinking that, will do.

Adds a feature flag that allows us to merge the paywalls component to
main but keep it out of any compiled packages. Avoids the pains of a
long-running feature branch.
@jamesrb1 jamesrb1 merged commit b70eef9 into paywalls-components/initial-merge Aug 21, 2024
4 of 5 checks passed
@jamesrb1 jamesrb1 deleted the paywalls-components/file-split branch August 21, 2024 17:14
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