fix(fxFlex): prevent setting min/max-size when grow/shrink is zero #160

somombo · 2017-02-07T00:09:36Z

handle various scenarios of grow/shrink being zero:

set min-width/min-height only if grow != 0
set max-width/max-height only if shink != 0
set both min and max if both grow == 0 AND shink == 0
add tests for these scenarios
remove max-width from test "should set a min-width when the shrink == 0"

fixes #153

Signed-off-by: Somo somo@mombo.solutions

joshwiens · 2017-02-07T00:33:54Z

@somombo - https://travis-ci.org/angular/flex-layout/jobs/199037704

somombo · 2017-02-07T00:35:19Z

@d3viant0ne i'm on it, had forgotten to run the linter

ThomasBurleson

Code formatting/logic change.

ThomasBurleson · 2017-02-07T01:01:43Z

src/lib/flexbox/api/flex.ts

@@ -301,8 +311,21 @@ export class FlexDirective extends BaseFxDirective implements OnInit, OnChanges,
    let usingCalc = String(basis).indexOf('calc') > -1;
    let isPx = String(basis).indexOf('px') > -1 || usingCalc;

-    css[min] = (basis == '0%') ? 0 : isPx ? basis : null;
-    css[max] = (basis == '0%') ? 0 : usingCalc ? null : basis;


Plz use this:

// if shrink and grow are both zero then make box inflexible // - should not set a min when the grow is zero // - should not set a max when the shrink is zero let isFixed = !grow && !shrink; css[min] = (basis == '0%') ? 0 : isFixed || (isPx && grow) ? basis : null; css[max] = (basis == '0%') ? 0 : isFixed || (!usingCalc && shrink) ? basis : null;

Done.
Though it does look a bit awkward.

No Problem.
Thanks for looking over so quick.

That's honestly a matter of perspective. You can get a lot evaluated, very concisely using a ternary and usually on a single line.

Once you get used to scanning it, the if ( ) conditions start to become a bit tedious to read / write.

Sure, as long we get to a place where #153 and things stemming from it are resolved. I really done care much about the minor details..

Thanks for all the feadback!

ThomasBurleson · 2017-02-07T02:40:49Z

@somombo - just an FYI that you will need to rebase your PR tomorrow after #158 is merged into master. At that point I can finish testing your PR and update to presubmit-for-merge.

From a code perspective, these changes look good. Testing with the demos will reveal any issues.

Thx for your efforts on this PR.

somombo · 2017-02-07T02:54:09Z

@ThomasBurleson - Yeah, no problem! I tested with demos and everthing looked identical to before.

By the way, none of the demos exemplify usage of something like 1 0 123px or 0 1 123px. It may not be a bad idea to provide such examples in future.

ThomasBurleson · 2017-02-07T03:50:46Z

@somombo - want to prepare a cool demo that highlights why your changes are useful ? That would be a great addition to this PR IMO.

somombo · 2017-02-07T20:53:11Z

@ThomasBurleson this is now rebased

ThomasBurleson · 2017-02-07T22:04:38Z

@somombo - Can you squash plz. Your change list looks huge for some reason.

somombo · 2017-02-07T22:23:16Z

@ThomasBurleson could you post the commit hashes for the extraneous commits just so i know which ones you're referring to?

googlebot · 2017-02-07T22:23:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

somombo · 2017-02-07T22:36:01Z

(@googlebot) I had changed my cla and my git email to somo@mombo.solutions for consistency. That someone was me in both cases

ThomasBurleson · 2017-02-07T22:43:42Z

@somombo - Your change list appears to include this commit SHA dad69fe that is already in master.

somombo · 2017-02-07T22:54:54Z

@ThomasBurleson Okay I see what you are referring to now.. I'm not sure how to fix that, kindly advise

ThomasBurleson · 2017-02-07T23:14:02Z

try:

git co somombo/issue153_maxmin_patch;
git pull --rebase upstream master;
git push -f origin somombo/issue153_maxmin_patch

This assume you have registered upstream as a remote branch to https://github.com/angular/flex-layout.git

somombo · 2017-02-07T23:22:35Z

@ThomasBurleson Appears to not have done anything. Here is what I got for those three commands respectively:

Already on 'somombo/issue153_maxmin_patch'
Current branch somombo/issue153_maxmin_patch is up to date.
Everything up-to-date

ThomasBurleson · 2017-02-08T00:06:06Z

@somombo - do this:

get rebase -i HEAD~3,
then d to remove the commit dad69fe
then exit and save
get push -f origin issue153_maxmin_patch

googlebot · 2017-02-08T08:37:26Z

CLAs look good, thanks!

ThomasBurleson · 2017-02-08T14:10:50Z

@somombo - I added you as a contributor to this repository.

Future PRs should be based on branch from this PR; which allows the core team easier access to your changes.

ThomasBurleson · 2017-02-08T18:10:41Z

@here - CLA was signed.

somombo · 2017-02-08T19:33:28Z

@ThomasBurleson Thanks! I appreciate it..

Does that mean for future PRs n patches, I should just work with the master branch on my fork.. rather than creating a somombo/... branch ?

somombo · 2017-02-08T19:33:45Z

@here sorry accidentally hit the "close pull request" button instead of the comment button

ThomasBurleson · 2017-02-08T23:24:54Z

@somombo - apologies, but a recent merge to master requires yet another rebase to this PR.

handle various scenarios of grow/shrink being zero: * set `min-width`/`min-height` only if `grow != 0` * set `max-width`/`max-height` only if `shink != 0` * set both min and max if both `grow == 0` AND `shink == 0` * add tests for these scenarios * remove max-width from test "should set a min-width when the shrink == 0" fixes angular#153 Signed-off-by: Somo <somo@mombo.solutions>

googlebot · 2017-02-09T00:19:34Z

CLAs look good, thanks!

somombo · 2017-02-09T00:24:18Z

@ThomasBurleson is this good?

somombo · 2017-02-09T00:46:14Z

@ThomasBurleson so what do I do next, do I hit the "squash and merge" or the "rebase and merge" button?

ThomasBurleson · 2017-02-09T01:26:52Z

@somombo - you should NEVER hit the Merge button(s). That is for the Core Team Admins only.

I will start the presubmit testing and merge process. THx

somombo · 2017-02-09T01:41:45Z

@ThomasBurleson I figured if there was something I wasn't supposed to do, the system would be set up so I wouldn't be able to do it.. but wasn't sure so good thing I asked :)

ThomasBurleson · 2017-02-09T01:44:50Z

@somombo - master is protected and accessible only to admins. you have write capabilities so you can create branches and update issues, assign labels, etc.. Just do not merge ;-).

…ngular#160) handle various scenarios of grow/shrink being zero: * set `min-width`/`min-height` only if `grow != 0` * set `max-width`/`max-height` only if `shink != 0` * set both min and max if both `grow == 0` AND `shink == 0` * add tests for these scenarios * remove max-width from test "should set a min-width when the shrink == 0" fixes angular#153 Signed-off-by: Somo <somo@mombo.solutions>

angular-automatic-lock-bot · 2019-09-05T22:02:57Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Feb 7, 2017

somombo force-pushed the somombo/issue153_maxmin_patch branch from faf3087 to f5409eb Compare February 7, 2017 01:00

ThomasBurleson reviewed Feb 7, 2017

View reviewed changes

somombo force-pushed the somombo/issue153_maxmin_patch branch from f5409eb to b8b5d1c Compare February 7, 2017 01:38

ThomasBurleson added in-progress pr: needs rebase labels Feb 7, 2017

ThomasBurleson mentioned this pull request Feb 7, 2017

fxFlex: is not configuring min-height properly for 'column' layouts. #162

Closed

somombo force-pushed the somombo/issue153_maxmin_patch branch 2 times, most recently from 07d14ad to 76e8bc2 Compare February 7, 2017 20:46

ThomasBurleson force-pushed the master branch from dad69fe to aaeb525 Compare February 7, 2017 21:28

googlebot added cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: yes labels Feb 7, 2017

somombo force-pushed the somombo/issue153_maxmin_patch branch from 76e8bc2 to b09eff2 Compare February 8, 2017 08:37

googlebot removed the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Feb 8, 2017

ThomasBurleson added pr: lgtm This PR has been approved by the reviewer pr: needs presubmit and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ in-progress pr: needs rebase labels Feb 8, 2017

somombo closed this Feb 8, 2017

somombo reopened this Feb 8, 2017

tinayuangao added the pr: needs rebase label Feb 8, 2017

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Feb 9, 2017

somombo force-pushed the somombo/issue153_maxmin_patch branch from 028cf4b to 830b968 Compare February 9, 2017 00:19

googlebot added cla: yes and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Feb 9, 2017

ThomasBurleson removed the pr: needs rebase label Feb 9, 2017

tinayuangao merged commit 942939e into angular:master Feb 9, 2017

somombo mentioned this pull request May 7, 2017

Setting flex-basis to 0 results in max-width: 0 #277

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fxFlex): prevent setting min/max-size when grow/shrink is zero #160

fix(fxFlex): prevent setting min/max-size when grow/shrink is zero #160

somombo commented Feb 7, 2017

joshwiens commented Feb 7, 2017

somombo commented Feb 7, 2017

ThomasBurleson left a comment

ThomasBurleson Feb 7, 2017

somombo Feb 7, 2017

joshwiens Feb 7, 2017 •

edited

Loading

somombo Feb 7, 2017

ThomasBurleson commented Feb 7, 2017 •

edited

Loading

somombo commented Feb 7, 2017

ThomasBurleson commented Feb 7, 2017 •

edited

Loading

somombo commented Feb 7, 2017

ThomasBurleson commented Feb 7, 2017

somombo commented Feb 7, 2017

googlebot commented Feb 7, 2017

somombo commented Feb 7, 2017 •

edited

Loading

ThomasBurleson commented Feb 7, 2017

somombo commented Feb 7, 2017

ThomasBurleson commented Feb 7, 2017 •

edited

Loading

somombo commented Feb 7, 2017 •

edited

Loading

ThomasBurleson commented Feb 8, 2017 •

edited

Loading

googlebot commented Feb 8, 2017

ThomasBurleson commented Feb 8, 2017

ThomasBurleson commented Feb 8, 2017

somombo commented Feb 8, 2017

somombo commented Feb 8, 2017 •

edited

Loading

ThomasBurleson commented Feb 8, 2017

googlebot commented Feb 9, 2017

somombo commented Feb 9, 2017

somombo commented Feb 9, 2017

ThomasBurleson commented Feb 9, 2017 •

edited

Loading

somombo commented Feb 9, 2017 •

edited

Loading

ThomasBurleson commented Feb 9, 2017

angular-automatic-lock-bot bot commented Sep 5, 2019

fix(fxFlex): prevent setting min/max-size when grow/shrink is zero #160

fix(fxFlex): prevent setting min/max-size when grow/shrink is zero #160

Conversation

somombo commented Feb 7, 2017

joshwiens commented Feb 7, 2017

somombo commented Feb 7, 2017

ThomasBurleson left a comment

Choose a reason for hiding this comment

ThomasBurleson Feb 7, 2017

Choose a reason for hiding this comment

somombo Feb 7, 2017

Choose a reason for hiding this comment

joshwiens Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

somombo Feb 7, 2017

Choose a reason for hiding this comment

ThomasBurleson commented Feb 7, 2017 • edited Loading

somombo commented Feb 7, 2017

ThomasBurleson commented Feb 7, 2017 • edited Loading

somombo commented Feb 7, 2017

ThomasBurleson commented Feb 7, 2017

somombo commented Feb 7, 2017

googlebot commented Feb 7, 2017

somombo commented Feb 7, 2017 • edited Loading

ThomasBurleson commented Feb 7, 2017

somombo commented Feb 7, 2017

ThomasBurleson commented Feb 7, 2017 • edited Loading

somombo commented Feb 7, 2017 • edited Loading

ThomasBurleson commented Feb 8, 2017 • edited Loading

googlebot commented Feb 8, 2017

ThomasBurleson commented Feb 8, 2017

ThomasBurleson commented Feb 8, 2017

somombo commented Feb 8, 2017

somombo commented Feb 8, 2017 • edited Loading

ThomasBurleson commented Feb 8, 2017

googlebot commented Feb 9, 2017

somombo commented Feb 9, 2017

somombo commented Feb 9, 2017

ThomasBurleson commented Feb 9, 2017 • edited Loading

somombo commented Feb 9, 2017 • edited Loading

ThomasBurleson commented Feb 9, 2017

angular-automatic-lock-bot bot commented Sep 5, 2019

joshwiens Feb 7, 2017 •

edited

Loading

ThomasBurleson commented Feb 7, 2017 •

edited

Loading

ThomasBurleson commented Feb 7, 2017 •

edited

Loading

somombo commented Feb 7, 2017 •

edited

Loading

ThomasBurleson commented Feb 7, 2017 •

edited

Loading

somombo commented Feb 7, 2017 •

edited

Loading

ThomasBurleson commented Feb 8, 2017 •

edited

Loading

somombo commented Feb 8, 2017 •

edited

Loading

ThomasBurleson commented Feb 9, 2017 •

edited

Loading

somombo commented Feb 9, 2017 •

edited

Loading