Skip to content

Conversation

@lissavxo
Copy link
Collaborator

Related to #230

Description

Added size prop, the size config will apply a scale to the button making it smaller or larger. Available options are 'xs', 'sm', 'md'(default), 'lg', 'xl'.

Test plan

Test size prop options 'xs', 'sm', 'md', 'lg', 'xl'

@Klakurka Klakurka requested review from Klakurka and chedieck April 21, 2025 15:29
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

  • In paybutton-generator.html, can you make the 'Size' input a dropdown?
  • For now, let's keep the button size inside the dialog/widget the same. Later on, we can potentially do a v2 that makes those "smaller" in some ways but for now I think it just makes more sense to leave them as is.

@lissavxo lissavxo requested a review from Klakurka April 21, 2025 16:31
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

  • The actual container div size needs to be adjusted too (both bigger or smaller as necessary): image
  • How about adding some full-word aliases? eg. "extrasmall", "small", "medium", "large", "extralarge".

@lissavxo lissavxo requested a review from Klakurka April 28, 2025 15:55
Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

Can you elaborate why this props is passed all the way down to PaybuttonDialog, WidgetContainer and Widget? I ask because the affected button exists before these components, and not inside them.

};
};

export type buttonSize = 'xs' | 'sm' | 'md' | 'lg' | 'xl' | "extrasmall" | "small" | "medium" | "large" | "extralarge" | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

UpperCamelCase here, since it refers to a type

@lissavxo
Copy link
Collaborator Author

lissavxo commented May 5, 2025

Can you elaborate why this props is passed all the way down to PaybuttonDialog, WidgetContainer and Widget? I ask because the affected button exists before these components, and not inside them.

This way, I can control which button receives the size prop config. Only the PayButton outside the dialog and widget will receive the size prop.

@lissavxo lissavxo force-pushed the feat/size-prop branch 3 times, most recently from d66b8b9 to 2bab76f Compare May 5, 2025 19:54
chedieck
chedieck previously approved these changes May 8, 2025
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

  • The div size is still the same no matter what button size is chosen (same point I made before).

@chedieck
Copy link
Collaborator

I think this is more complicated than it first seemed. To change the size, it's necessary to modify paybutton/src/index.tsx's Paybutton.render function, since it's the outer div (the one where the paybutton is rendered inside) that needs to have its style modified.

@lissavxo lissavxo requested review from Klakurka and chedieck May 28, 2025 16:55
Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

Conflicts

@lissavxo
Copy link
Collaborator Author

resolved

@lissavxo lissavxo requested a review from chedieck May 29, 2025 18:26
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Doesn't build for me:

image

@lissavxo lissavxo requested a review from Klakurka June 11, 2025 16:24
@lissavxo
Copy link
Collaborator Author

Doesn't build for me:

image

Should be okay now

Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

would be possible to change the outer container someway other than by using transform?
The problem with transform is that it overlaps the outer stuff:
image

(tested using https://jsfiddle.net/ktu0q4o1, if you wanna experiment there)

Here is the same thing happening in PayButton (I added blue background color to the outer <div class="paybutton">, to show how it expands without pushing stuff in the side)
image

@lissavxo
Copy link
Collaborator Author

would be possible to change the outer container someway other than by using transform? The problem with transform is that it overlaps the outer stuff: image

(tested using https://jsfiddle.net/ktu0q4o1, if you wanna experiment there)

Here is the same thing happening in PayButton (I added blue background color to the outer <div class="paybutton">, to show how it expands without pushing stuff in the side) image

Now we are just changing the font size instead of transform

@chedieck
Copy link
Collaborator

Now we are just changing the font size instead of transform

I see, and this apparently expands the button size much like transform did.
Changing with transform:
image

Changing with font-size only:
image

There are some differences, but overall it expands the area that the button occupies without making the neighboring content adjust to these new sizes.

If there were a way to e.g. change the div's width instead of transforming it or only increasing the font-size, that would make the sibling HTML tags actually consider that space as "taken" and show buttons that don't overlap

I tried to make this work but failed, maybe @johnkuney can help us make this work?

@johnkuney
Copy link
Collaborator

For sure, I may not be understanding the issue exactly, but I think it's more a problem with the grid css in our index.html file.

The grid columns aren't really responsive to the content width, it's instead fitting as many columns it can that are at least 180px on each row. So if the content is larger it overflows and if its its smaller it doesnt adjust. At least I think thats whats happening

If we move to a flexbox layout everything works as expected. Hope you dont mind, I'll push some tweaks I made to make our preview file layout work, I also added some adjustments to the border radius so it scales with the button size as I thought that was a bit better than 10px on every size

Klakurka
Klakurka previously approved these changes Jun 19, 2025
@Klakurka
Copy link
Member

Conflicts.

@chedieck chedieck self-requested a review June 23, 2025 14:22
@chedieck chedieck merged commit 74d4038 into master Jun 23, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants