Size on Buttons

[dickermoshe]

I understand that this is in the early stages of development, but I had a thought. Let me know if we should leave you alone during this early phase

Setting the size using ShadcnButtonSize is ignored if a height is set on the inherited theme. Arguments on a widget itself should take precedence over the inherited theme.

https://github.com/nank1ro/shadcn-ui/blob/9c3278988be3160d2dbce226b50f7f943a1827f2/lib/src/components/button.dart#L286-L310

P.S. It would be awesome if the preset values could be inherited from the Theme.

[nank1ro]

Hi there, thanks for reporting the issue. Just refactored the whole theme, because it didn't convince me. I think how I implemented it now should be better.

I also changed the order in which the operations are performed to compute the size of the button, check https://github.com/nank1ro/shadcn-ui/blob/main/lib/src/components/button.dart#L298C3-L307C6

I also moved some of the presets in the component theme. Let me know what you think about it

[dickermoshe]

A padding that doesn't work when a size is declared seems wrong. By default, the theme has a size of $default , so the padding declared on the theme won't work unless size is explicitly set to null. IMO I would expect an explicitly declared padding theme to take precedence over a default size.

https://github.com/nank1ro/shadcn-ui/blob/1b02df7abe95f87555d7577a3711c1991c6afcbd/lib/src/components/button.dart#L346-L355

https://github.com/nank1ro/shadcn-ui/blob/1b02df7abe95f87555d7577a3711c1991c6afcbd/lib/src/theme/components/button.dart#L14

Also, these button sizes aren't customizable at all.

My opinion: Kill 2 birds with one stone: Create a ButtonSizesTheme with sm, lg, icon, and default with a ButtonSize class that has height, width and padding

ButtonSizesTheme(sm:ButtonSize(height:50));

[dickermoshe]

I would go with whatever Material Mike has to say about implementing a ThemeData, he's done this the most. Thanks for being so gracious ❤️

[nank1ro]

Would you still have the single properties width, height and padding in the Button class? These properties would be removed from the button theme and converted in this single class?

Feel free to open a PR if you have all in mind 💙 I personally don't like to have multiple sources of truth, for example the padding property is determined by both the padding and size properties, while it should be based on a single source of truth.

[dickermoshe]

Would you still have the single properties width, height and padding in the Button class?

Yeah, using another class would here would seem strange.

These properties would be removed from the button theme and converted in this single class?

Yup

I personally don't like to have multiple sources of truth, for example the padding property is determined by both the padding and size properties, while it should be based on a single source of truth.

Agree

See what I did here: https://github.com/nank1ro/shadcn-ui/pull/2

Right now the size property only affects width, height and padding. So that's what I moved into another theme.

The issue with my implementation is that I could see how people would want to move other things into a subclass: E.G. If a user want to have more rounded corners on the large button.

Another way could be having the entire main theme contain 4 different button themes. This would make things much more complex, we would basically need to do what you've done with the types of buttons (primary, destructive...) for sizes too.

[dickermoshe]

Another way: Similar to materialstatecontroller

var width = ShadcnButtonSizeProperty.resolveWith((size) {
  return switch (size) {
      ShadcnButtonSize.$default => 40,
      ShadcnButtonSize.sm => 36,
      ShadcnButtonSize.lg => 44,
      ShadcnButtonSize.icon => 40,
    };
},);

I've implemented this on another branch. Let me know if you would rather this.

[nank1ro]

Another way:

Similar to materialstatecontroller

var width = ShadcnButtonSizeProperty.resolveWith((size) {

  return switch (size) {

      ShadcnButtonSize.$default => 40,

      ShadcnButtonSize.sm => 36,

      ShadcnButtonSize.lg => 44,

      ShadcnButtonSize.icon => 40,

    };

},);

I've implemented this on another branch.

Let me know if you would rather this.

Feel free to open another PR, I'll review both this morning

[nank1ro]

closed by https://github.com/nank1ro/shadcn-ui/pull/2