Added support for min/max width and height constraints. #65

2015-03-31T02:42:53-07:00

freakboy3742 commented

2015-03-31 02:42:53 -07:00

(Migrated from github.com)

This patch adds initial support for minWidth, maxWidth, minHeight and maxHeight CSS constraints.

It contains tests which cover a good proportion of "normal" uses of min/max constraints. However, there are still a lot of untested combinations, especially in the realm of poorly specified constraints - e.g., a minHeight value that is greater than maxHeight. These cases are common in the random test suite, but almost never happen In Real Life.

This patch adds initial support for minWidth, maxWidth, minHeight and maxHeight CSS constraints. It contains tests which cover a good proportion of "normal" uses of min/max constraints. However, there are still a lot of untested combinations, especially in the realm of poorly specified constraints - e.g., a minHeight value that is greater than maxHeight. These cases are common in the random test suite, but almost never happen In Real Life.

vjeux commented

2015-03-31 08:39:09 -07:00

(Migrated from github.com)

Omg, this is so good!

Do you have plans to continue working on this to get as close as possible to the specification? The edge cases are actually pretty important, this is what will make React Native able to run on web. It also makes my life easier as I can just redirect people to the browser/css spec when they have an issue.

Omg, this is so good! Do you have plans to continue working on this to get as close as possible to the specification? The edge cases are actually pretty important, this is what will make React Native able to run on web. It also makes my life easier as I can just redirect people to the browser/css spec when they have an issue.

mrjjwright commented

2015-03-31 10:33:42 -07:00

(Migrated from github.com)

Yay!!

freakboy3742 commented

2015-03-31 16:44:58 -07:00

(Migrated from github.com)

Unless I hit a bug in practice, I'm not expecting to submit another pull request in the next week or so. Longer term, I expect I'll revisit that - a 100% complete implementation of the CSS box model is just too sweet a target for my left brain to ignore :-)

I submitted what I had because it seemed to be working for all the common cases, and I was hitting a point of diminishing returns tracking down edge cases. Part of the problem is that some of the browser behaviours I was seeing didn't appear to make sense, and were inconsistent between browsers, and it wasn't clear if that was a problem with specific browser implementations.

What I really need is an analog of the old ACID2 test, but for flexbox - a complex and spanning demonstration of the CSS box model that has been vetted for correctness.

Unless I hit a bug in practice, I'm not expecting to submit another pull request in the next week or so. Longer term, I expect I'll revisit that - a 100% complete implementation of the CSS box model is just too sweet a target for my left brain to ignore :-) I submitted what I had because it seemed to be working for all the common cases, and I was hitting a point of diminishing returns tracking down edge cases. Part of the problem is that some of the _browser_ behaviours I was seeing didn't appear to make sense, and were inconsistent between browsers, and it wasn't clear if that was a problem with specific browser implementations. What I really need is an analog of the old ACID2 test, but for flexbox - a complex and spanning demonstration of the CSS box model that has been vetted for correctness.

vjeux commented

2015-03-31 16:52:09 -07:00

(Migrated from github.com)

I've been making bug reports on the browsers for behaviors that seemed completely wrong. Would be nice to do the same and add those in comments.

I also hope that the tests in this repo will help make a corpus of compliance tests if anyone wants to build another layout implementation

I've been making bug reports on the browsers for behaviors that seemed completely wrong. Would be nice to do the same and add those in comments. I also hope that the tests in this repo will help make a corpus of compliance tests if anyone wants to build another layout implementation

subtleGradient commented

2015-03-31 19:37:12 -07:00

(Migrated from github.com)

💰 💰 💰

:moneybag: :moneybag: :moneybag:

glenjamin commented

2015-03-31 23:49:51 -07:00

(Migrated from github.com)

Does the maxWidth/maxHeight still kick in on boxes with a non-zero flex value?

Do these properties mean flexBasis is unnecessary?

Does the maxWidth/maxHeight still kick in on boxes with a non-zero flex value? Do these properties mean flexBasis is unnecessary?

freakboy3742 commented

2015-04-01 02:22:03 -07:00

(Migrated from github.com)

Yes, maxWidth/maxHeight applies to boxes with non-zero flex values. The max/min width will override any flex weight-based allocation of width, effectively removing the element from the flex calculation.

I suppose flexBasis could be used as an alternative to minWidth/Height in most common scenarios; but I'm guessing there will be some subtle edge cases where they don't give the same result.

Yes, maxWidth/maxHeight applies to boxes with non-zero flex values. The max/min width will override any flex weight-based allocation of width, effectively removing the element from the flex calculation. I suppose flexBasis could be used as an alternative to minWidth/Height in most common scenarios; but I'm guessing there will be some subtle edge cases where they don't give the same result.

vjeux commented

2015-04-06 15:29:49 -07:00

(Migrated from github.com)

I took a closer look at there are some legitimate use cases that are not working right now and should.

  it('should layout minHeight with a flex child', function() {
    testLayout(
      {style: {minHeight: 800}, children: [
        {style: {flex: 1}}
      ]},
      {width: 0, height: 800, top: 0, left: 0, children: [
        {width: 0, height: 800, top: 0, left: 0}
      ]}
    );
  });

In the current implementation, the height of the first child is 0.

We really need to fix most of the edge cases before using it. Otherwise, people are going to be super confused and think that it's broken :(

I took a closer look at there are some legitimate use cases that are not working right now and should. ``` javascript it('should layout minHeight with a flex child', function() { testLayout( {style: {minHeight: 800}, children: [ {style: {flex: 1}} ]}, {width: 0, height: 800, top: 0, left: 0, children: [ {width: 0, height: 800, top: 0, left: 0} ]} ); }); ``` In the current implementation, the height of the first child is 0. We really need to fix most of the edge cases before using it. Otherwise, people are going to be super confused and think that it's broken :(

freakboy3742 commented

2015-04-06 20:05:55 -07:00

(Migrated from github.com)

I've just added PR #68 that corrects this problem.

The other test introduced in b912acf is another matter; I'll leave a comment on that commit.

I've just added PR #68 that corrects this problem. The other test introduced in b912acf is another matter; I'll leave a comment on that commit.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#65