[ASLayoutSpec] Set flexShrink = YES by default on ASTextNode.

[appleguy]

No description provided.

[appleguy]

It is a hard question, but do you think this is net beneficial? @rcancro @nguyenhuy @eanagel @levi @vitalybaev - note that I needed to install the flexShrink = NO cases to make the SocialAppLayout example layout correctly.

However, I think the more common case is to have a text node in a vertical stack that needs to wrap, or a text node in a horizontal stack with images and it needs to wrap (with the images staying their preferred size). The tricky cases come up only when you have multiple text nodes in the same HORIZONTAL stack, and in that case users would have to set flexShrink = YES on some anyway; this changes it so that they would need to set flexShrink = NO. My hope is that it is net beneficial in that it reduces the total number of flexShrink sets in a given app, and especially for users' first attempt using the tools so they don't feel overwhelmed by all the options + ultimately get stuck without the layout working how they intend.

The line here with the real name, username, and time node is an interesting example:

[rcancro]

I wonder if the strange side effects that you can get from setting all text nodes to flexShrink = YES are worse than seeing text overflow out of the view? Perhaps we could add an assert/log in a horizontal stack that warns when its children cannot be sized to fit in the constrained size. The message could suggest adding flexShrink = YES to a node. This is actually kind of similar to what UIKit does with autolayout when constraints conflict.

[smiLLe]

I had to set flexShrink = YES in almost all Components in ComponentKit in horizontal stacks. However and afaik, css flexbox also requires you to set flexShrink = YES in this case. Also flexShrink and flexGrow are meant to be int values rather than just a bool.
see this comment facebook/componentkit#333 (comment)

IMO set flexShrink to YES, as it is the behaviour i expect as a beginner. However me move further away from the css spec

[nguyenhuy]

Checking the CSS flex-shrink spec, the default value of flexShrink should be YES indeed. While I think we should change the default value to YES (for all layoutable objects, maybe?), we need to consider the negative effects such change would cause for existing users. The very least thing we can do is mentioning this (breaking?) change clearly in the release note and suggest people to revisit their layouts.

@smiLLe: Thanks for the heads-up and mentioning the CK issue. Changing flexGrow and flexShrink to integers and supporting proportionally shrinking/growing would be useful (but breaking) changes. It would be great if we have a separate issue for this matter. I think we should aim for v2.1?

[nguyenhuy]

And I do think that a sensible default value for flexShrink (probably YES) will greatly improve the experience of developers new to the layout system.

[appleguy]

@nguyenhuy thanks for your comments - they're very valuable to this important decision.

Is the reason the CSS spec says that because it considers flex-shrink a rather than a boolean? Maybe 1 is essentially the same as NO / no-op?

As far as affecting current users — I think this is a change we'd make in the 2.0 final version, and indeed this is one of the things that I've emphasized with 1.9 (that the ASLayoutSpec APIs may see changes requiring authors to make modifications, although we did promise that the updates wouldn't be technically difficult to make). I'm comfortable with very highly emphasizing this in release notes so that there is no ambiguity around the cause of any behavior changes for updaters. We may even roll a 2.0 Beta first to get the first pass of people experiencing this to see how disruptive it is, and try to spread the word.

@levi, @Adlai-Holler, @rcancro, I'd really appreciate your weighing in on this call!

[levi]

Yeah, this change is sure to break a few things for us. Not to worry, however, we're flexible with these kind of changes, since the API is still in flux.

[Adlai-Holler]

Don't have much to throw in – I totally agree it's a sensible default for all nodes, it won't affect us very much, and even when it affects other users the results will be 1) easy-to-notice for the most part and 2) not catastrophic so I don't think it's unreasonable to include in a major version bump. Especially if the CSS spec recommends it as the default.

[nguyenhuy]

@appleguy A flex-shrink integer specifies the shrink factor of a child, which determines how much it will shrink relative to all other shrinkable children. If all shrinkable children have the same flex-shrink value, negative free space will be evenly distributed among them. So for current users, flexShrink = YES essentially means flexShrink = 1, which is the default value. A flex-shrink of 0 means not-shrinkable (thus equals flexShrink = NO). Negative values are invalid, and so NSUInteger seems like the correct data type.

Original spec: http://www.w3.org/TR/css-flexbox-1/#flex-shrink-factor
Easier-to-understand explanation: https://scotch.io/tutorials/a-visual-guide-to-css3-flexbox-properties#flex-shrink

Similar behaviour for flex-grow integer.

If we decide to implement flex-shrink and flex-grow integers, most of the work will be about implementing proportionally shrinking and growing behaviours.

[appleguy]

@nguyenhuy wow, very interesting! I guess we should probably do this, then.

Question - is it not logically possible for flexShrink to be a floating point number? This is just a curiosity I have about the spec...I don't think it's a priority to implement numeric flexShrink / flexGrow in ASDK for the next several months.

[nguyenhuy]

@appleguy Indeed, the spec states that those flex values are floating points. Could you please create a new issue to track this feature, with an appropriate milestone (2.1?).

And if you close this PR, it means you agree that flexShrink should be enabled by default for all nodes. I can make a new PR to do just that soon afterward :) The diff will be small and should be in the coming release I guess.

[aaronschubert0]

@appleguy Will we just merge this in time for 2.0? I think everyone is in favour for this behaviour, and would be good to get in as more people make use of this framework.

[appleguy]

@aaronschubert0 @rcancro @nguyenhuy @levi @maicki okay, this change is rebased. I suppose we should land it soon, but perhaps after 1.9.8? Could anyone at Pinterest try it out and see how many layouts need to be revised?

[maicki]

@appleguy @nguyenhuy Why do we need to add this property to ASTextNode? It's already in ASLayoutable.h

[maicki]

I set flexShrink to YES by default within Pinterest and browsed a bit around and it seems like on the first look nothing broke.

I think we should make a call here and either merge it for master soon or close it.

@appleguy @levi @nguyenhuy Any doubts / thoughts merging it before 2.0?

[nguyenhuy]

I'd vote for merging it soon.

[aaronschubert0]

I'm happy to merge it in soon also.

[maicki]

Hey @appleguy can you please rebase the branch on top of latest master. We will likely merge that pretty soon. Thanks!

[maicki]

@Adlai-Holler @appleguy @levi I would like to bring that PR up again. I think we should not just move the ASTextNode to have flexShrink enabled by default, we should enable it by default on all ASLayoutable's.

This would follow the CSS standard: https://drafts.csswg.org/css-flexbox-1/#propdef-flex-shrink

[Adlai-Holler]

This discussion is very valuable, but I expect the diff to be superseded by #2071