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

Adds scaffolding for the ButtonComponent. #4348

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

Conversation

JayShortway
Copy link
Member

@JayShortway JayShortway commented Oct 3, 2024

As the title says. Just creates the files I think are necessary, with very minimal content. How the ButtonComponent is going to interplay with the existing LinkButtonComponent and upcoming PurchaseButtonComponent can be seen later imo, but let me know your thoughts!

@JayShortway JayShortway requested review from a team October 4, 2024 12:17
@JayShortway JayShortway marked this pull request as ready for review October 4, 2024 12:17
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.

Yay paywall components PR 🥳

One change request on not composing with TextComponent (which I'll take credit that LinkButton was a bad example and I was going to delete it soon)

struct ButtonComponent: PaywallComponentBase {

let type: ComponentType
public let text: PaywallComponent.TextComponent
Copy link
Member

Choose a reason for hiding this comment

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

This looks similar to the LinkButton where it has aTextComponent inside of it (which makes sense) but I totally forgot to delete the LinkComponent and mention when why it was build that way 😅 It was built for very early demo purpose and just reused TextComponent because easy.

However, I think we are going to be leaning towards more of an protocol style instead of composition for components that have similar things here.

I started doing this with the StackableComponent in https://github.com/RevenueCat/purchases-ios/pull/4324/files#diff-ec1da30bf93d1075c4c1985847a4762e27386eb3a6cb81a81dee288d50ba2fcc

So here, we could create a new protocol called like TextableComponent (names are hard???) that have the properties that are similar between TextComponent and ButtonComponent.

My reason for this is:

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense! However, are we going to allow people to give buttons arbitrary content, like SwiftUI and Jetpack Compose do? What about icons/images?

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... a great question 🤔 In the current spec, the button only has text. However...

Maybe a ButtonComponent should really be a container/stack and hold a list of components... and then the dashboard component selector can give options for like TextButtonComponent or IconButtonComponent that adds a button with those sub components in it already as a template?

Does that make sense? If so... we should bring this up to the Dan and Barbara 😇

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea that does make sense! And then things like "leading icon" vs "trailing icon" are just the same (horizontal) stack, but in a different order. I'll bring it up!

Maybe a ButtonComponent should really be a container/stack

Yea, or it could have a single StackComponent child?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe making buttons behave like stacks (in whatever implementation) might be good to do regardless, to not limit our future selves. 🤔

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.

Making the button behave like a stack looks good! Just one major question on the implementation of the stack here vs https://github.com/RevenueCat/purchases-ios/pull/4324/files#diff-6fed0f670d26d3bdf79586129d096238c544e4022a76f86a096997b387e6764dR22-R34

Comment on lines +22 to +34
struct ButtonComponent: PaywallComponentBase {

let type: ComponentType
public let stack: PaywallComponent.StackComponent

public init(
stack: PaywallComponent.StackComponent
) {
self.type = .button
self.stack = stack
}

}
Copy link
Member

Choose a reason for hiding this comment

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

So in my packages PRs, I have that StackableComponent protocol (see here) that does a similar thing to this 🤔

But instead of the ButtonComponent having a stack, it would make the ButtonComponent behave like a stack.

I know we don't StackableComponent in this branch yet but we should probably make sure we do the same approach for all the things 😇

I do think this having a stack looks a lot cleaner in the code! 🫶 However, it does make the JSON data structure of the ButtonComponent more nesty...

{
  "type": "button",
  # button properties
  "stack": {
    "type": "stack",
    "components": [
      # children components
    ]
    # all the stack properties
  }
}

vs

{
  "type": "button",
  "components": [
      # children components
    ],
   # button properties
   # all the stack properties
}

Any thoughts? 😇

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 yes, making it "have" a stack was a bit WIPpy to just get the idea out of my head and onto "paper" haha. Can definitely be changed still! If we want it to be a StackableComponent, I'll wait until your branch is merged.

I don't have a very strong opinion on this, but I do have some counter arguments we might wanna consider:

  • Composition is how both SwiftUI and Jetpack Compose build up view hierarchies. It might be good for us to stay close to this philosophy, to easily translate our components into views without a risk of having to "fight" the system.
  • JSON is primarily meant for machines to read, and code is mostly read by humans. If a change makes the code more readable and maintainable, but it makes the JSON more nested, that could be worth it?

What do you think?

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.

2 participants