feat(CupertinoButton): Add minWidth and minHeight to replace minSize. #161295
Conversation
github-actions
bot
added
framework
|
Can we also add a dart fix to map current usage of |
nate-thegrate
left a comment
nate-thegrate
left a comment
There was a problem hiding this comment.
I remember Justin mentioning that the material library's ButtonStyle has a minimumSize property, and it looks like its type is MaterialStateProperty<Size?>?.
And as Victor pointed out, we'll need a data-driven fix for this.
One thing that I'm wondering: is it possible that we'll introduce these new fields only to deprecate them in a few months?
We had plans to expand on our non-material theming system for more consistency across Material & Cupertino.
Maybe we should consider tabling this PR for now, and coming back to it once we decide whether CupertinoButton is going to have its own button style.
Thank you for your guidance. I’d be happy to add this, but I’m not very familiar with implementing this feature. I might need some time to learn about it. |
I’m not sure if I misunderstood your point. My understanding is that this doesn’t seem to conflict with my request. On the contrary, my request seems to align better with its standards. If I’m wrong, please let me know.
I believe standardizing the style should mean unifying the default style, but it shouldn’t prevent users from making modifications. I think users really need this property. I’m not sure what the future plans are, but I believe a button will certainly have these two properties.
I feel this request is very valuable. What should I do next? |
|
Apologies for my vague description earlier. It's possible that over the next few months, we'd decide on something like CupertinoButton(
style: CupertinoButtonStyle(
minWidth: 20.0,
minHeight: 10.0,
),
// ...
)If that ends up happening, I believe it'd be best to avoid introducing those fields to the widget constructor. |
|
I understand. Then I’ll wait for your work plan. Also, could I know where I can find the content of your ongoing discussions and plans? |
|
Sounds good, thanks! Unfortunately I'm not aware of any ongoing discussions about it. The extent of my knowledge is that we were planning to start in early 2025… so hopefully soon! |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
@dkwingsmt Given your upcoming work on theming/styling, do you think it makes sense to make a change like this now or to wait for those changes first?
| this.minSize, | ||
| @Deprecated( | ||
| 'Use minWidth and minHeight instead. ' | ||
| 'This feature was deprecated after v3.28.0-3.0.pre.', | ||
| ) | ||
| double? minSize, | ||
| double? minWidth, | ||
| double? minHeight, |
There was a problem hiding this comment.
What if instead of adding minWidth and minHeight we add minimumSize of type Size? That way it matches ButtonStyle.minimumSize.
There is risk that this will end up deprecated due to a refactor of theming as @nate-thegrate mentioned, but maybe it's less likely this way? Since it matches ButtonStyle.
There was a problem hiding this comment.
Your idea is fantastic! If this approach is acceptable, I’d be more than happy to adjust the functionality.
There was a problem hiding this comment.
I can't think of any big plans to change this part of theming in the near future, unless @/nate-thegrate has such as plan.
There was a problem hiding this comment.
Good to know, thank you Tong!
There was a problem hiding this comment.
What's the status of this proposal? Seems like a no-brainer to keep the API surface compact.
There was a problem hiding this comment.
Aside from the compact API surface, allowing a Size object could let us do something like
class WidgetStateSize extends Size implements WidgetStateProperty<Size?> {
...
}Unfortunately, changing a parameter type while keeping the same name would be difficult. And an advantage of the current approach is that you could specify a minimum width without setting a minimum height if you wanted, whereas a Size object requires both to be set.
Maybe we could rename minSize to minimumSize and hope that it doesn't confuse anybody :)
There was a problem hiding this comment.
API modifications indeed require great caution, and I also prefer adjusting the minimumSize parameter of the Size type.
I will make the necessary modifications.
There was a problem hiding this comment.
How about the concern raised by @nate-thegrate where the user can no longer leave width or height empty without specifying both? Are there real world use cases there that are not possible with this approach? Could we take double.infinity in those cases, or is that not user-friendly enough?
Sorry for going back and forth on this point so much :) Just trying to make sure we make a good decision on the API here.
There was a problem hiding this comment.
The back-and-forth is what I live for 😆
One option for maximum flexibility would be to accept a BoxConstraints object instead of a Size, but if a developer really wanted the fine-grain control they could just leave the minimumSize null and wrap the child with a ConstrainedBox.
I think a Size property would be great here!
There was a problem hiding this comment.
Alright I will renew my LGTM and leave it up to @StanleyCocos.
|
I'm ok with either adding |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
github-actions
bot
added
the
c: tech-debt
This comment was marked as resolved.
This comment was marked as resolved.
5 participants
Add minWidth and minHeight to CupertinoButton to facilitate control over the minimum dimensions.
fix: #161294
Pre-launch Checklist
///).