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

Add priority to MinimumSize and use it in ButtonStyle #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nataliq
Copy link
Contributor

@nataliq nataliq commented Apr 23, 2019

Specify the priority of a MinimumSize instance so that it can be used when setting up constraints.

It is useful to have the priority customizable so that through your ButtonStyle you can specify a minimum size for a button that gets respected in all cases. Currently I'm experiencing problems with cases where in stack views the minimum size won't be respected when with priority defaultHigh even when setting up compression resistance and hugging priority to required. This haven't been an issue so far because I was using content insets to specify the button size but now I'm switching to an absolute value that is not relative to the button title text size.

It's worth mentioning that we had discussion with @mansbernhardt about this before in the original PR introducing MinimumSize:
#67 (comment)

Note: I considered whether the priority should be set separately for width and height but this will lead to an API breaking change and there is no use case for it at the moment so I decided not to do it. If in the future there is a case for that, we can change it.

@nataliq nataliq requested a review from saidsikira April 23, 2019 18:08
@mansbernhardt
Copy link
Contributor

I still feel uncomfortable to add constraint information to styles. Could you give an example of when this might be useful and how it would be used? If added, it feels the style would be adjusted to or know about its context. But how would you know in beforehand how a ui-element using a style will be laid out?

Could perhaps this be better solved by exposing something like a minimunSizeConstraintsPriority on the UIButton itself?

@nataliq
Copy link
Contributor Author

nataliq commented Apr 24, 2019

Example is having buttons that have certain predefined intrinsic content size that doesn't allow them to become smaller in height/width than a fixed value.

There are few ways to achieve this that I know of, with content insets, with subclassing and overriding intrinsic content size (and I think few more things), and with constraints.

The first one is relative - you need to calculate the insets relative to the title font if you have an exact value, the second one needs a subclass and some calculations that can potentially be hard to maintain for different os versions, the 3rd one we already support with the MinimumSize but the priority defaultHigh is not flexible enough.

@mansbernhardt is your concern the type of the priority I'm using? If we want to abstract how it is implemented behind the scenes (say in the future we implement it with intrinsic content size or with a new property Apple adds) I can add a new type.. Otherwise the preferredMinimumSize of the ButtonStyle already captures some layout information and that's very convenient. Having minimunSizeConstraintsPriority on UIButton will mean that buttons with the same style can look quite differently, and if you want a different priority by default you will need to define a new UIButton convenient initializer that sets it. I'm quite open to consider it, I just need to understand better what's the downside of the solution implemented here as it looks more convenient to me.

@mansbernhardt
Copy link
Contributor

I'm worried that by adding a priority in one place it will cascade to other places as well and will complicate the styling. What priority is used should be based on the context (other constraint's priorities). Perhaps the problem is as you said, that we don't have access to all the tools (such as intrinsicSize) due to our wish to avoid subclassing (as this comes with its own problems).

My reading of preferredMinimumSize is that it's not a required minimum size but a preferred one. Hence the implementation to set the priority to .defaultHigh is perhaps what's broken to start with? Perhaps, it should rather be something lower such as .defaultHigh - 1´. (using -1, such as .required - 1` is used in other parts of Form to allow constraints to be broken based on priority).

.defaultHigh has this comment:

    public static let defaultHigh: UILayoutPriority // This is the priority level with which a button resists compressing its content.

What would the effect be to change the default constraint priority to `.defaultHigh - 1´, would that work?

Back to my initial worry. How would you as a designer of a reusable style know what to set the priority to? Isn't the priority chosen dependant on the the users of the style and how they set up the constraints around their button instances? But in reality, won't it be the other way around, the users of the style have to adjust to the style's priority (or locally override it, but then the priority is not really shared any longer). It is the user of the style that knows when its laid out buttons should allow to break their minimum style, hence the surrounding constraints will setup with priorities to fullfil the desired design.

But that means the the user of the style need to know what priority being used (and expect it not to change in the future). Therefore it makes more sense to have a fixed/constant priority (that should be documented), and select a the constant to a sensible value such as e.g. `.defaultHigh - 1´.

I'm not sure I'm making any sense here. But to summarise, the selection of constraints priorities are highly depending on the context where the element is being used and not something that is easily reused via a style. As a user of a UI element styled by a shared style, I should feel confident that my constraints work together (hence any hidden, none-configurable constraints should be documented if they affect constraints set up "outside"). The reason to not allow these to be configured via shared styles is that any change in a style's priority might break the constraints of its existing users.

@nataliq
Copy link
Contributor Author

nataliq commented Apr 24, 2019

Thanks a lot for the feedback! As you can see, I'm also not convinced how to solve this but I hope you understand the use case better now.

I understand what you're saying. Yes, preferredMinimumSize when named like this suggests that it can be broken where what I'm actually using it for is more like requiredMinimumSize. Do you have a suggestion how to handle this intent so that this is not allowed:

It is the user of the style that knows when its laid out buttons should allow to break their minimum style

To your other question:

How would you as a designer of a reusable style know what to set the priority to? Isn't the priority chosen dependant on the the users of the style and how they set up the constraints around their button instances?

The aim here is, if you as a user pass a style with required minimum size, to not have to worry about setting constraints to determine the size of the button. The same way as if you create a button with content insets [32, 16, 32, 16] you will get quite big button by default. You can stretch it (same with the minimum size which sets >= constraint, not ==) but you can't make it smaller. Maybe the difference is what happens when things break.

What would the effect be to change the default constraint priority to `.defaultHigh - 1´, would that work?

It will make preferredMinimumSize useless, it will never be used.

The reason to not allow these to be configured via shared styles is that any change in a style's priority might break the constraints of its existing users.

As I mentioned above I'm not sure if there is a difference between changing the minimum size value / priority and changing the button properties that contribute to its intrinsic content size (insets). I'm gonna test it now.

@mansbernhardt
Copy link
Contributor

What would the effect be to change the default constraint priority to `.defaultHigh - 1´, would that work?

It will make preferredMinimumSize useless, it will never be used.

Ok, what about the other way around, i.e. let the context (users of the buttons) that need the constraint to sometimes be broken to set their constraints to something above .defaultHigh?

@nataliq
Copy link
Contributor Author

nataliq commented Apr 24, 2019

I suspect you mean below .defaultHigh, not above (less than defaultHigh will allow this constraint to be broken).
That will be possible and work well provided we add some way of modifying the priority.

@mansbernhardt
Copy link
Contributor

@nataliq Would I mean is that you should let the buttons minimum size constraints priority to be fixed and publicly documented, and then let the context (constraints set between the buttons and other UI elements) either increase or decrease their priorities to get the expected effect.

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.

3 participants