Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(layout): allow layout-align without cross-axis value - #6003

Closed
ngraef wants to merge 1 commit into
angular:masterfrom
ngraef:fix-layout-align
Closed

fix(layout): allow layout-align without cross-axis value#6003
ngraef wants to merge 1 commit into
angular:masterfrom
ngraef:fix-layout-align

Conversation

@ngraef

Copy link
Copy Markdown
Contributor
  • fix parsing issue to allow single value
  • add demo to test omitting second value

fixes#5996

* fix parsing issue to allow single value
* add demo to test omitting second value
fixesangular#5996
@ThomasBurleson

Copy link
Copy Markdown
Contributor

@ngraef - this is not ideal. Allowing a mainAxis-only is fine, but the style should always fallback to defaults:

  • main-axis === 'start'
  • cross-axis === 'stretch'

If you update the demos accordingly, I will merge.

Meanwhile, this is already in place in the generation-2 Layout engine coming in January.

@ThomasBurlesonThomasBurleson added this to the 1.0-rc6 milestone Dec 2, 2015
@ThomasBurlesonThomasBurleson self-assigned this Dec 2, 2015
@ngraef

Copy link
Copy Markdown
ContributorAuthor

@ThomasBurleson I'm confused about what needs work. This doesn't change anything about the default values or the fallback behavior during parsing.

@ThomasBurleson

Copy link
Copy Markdown
Contributor

@ngraef - This PR is incomplete:

  • Layout.js needs updated with layout-align improvements for fallbacks/defaults
  • $scope.layoutAlign should not be checking for a blank crossAxis (that should be done in fallbacks).

I will curl your PR and fix.

@ngraef

Copy link
Copy Markdown
ContributorAuthor

@ThomasBurleson

  • layout.js already has a fallback to default values in extractAlignAxis:

    if(ALIGNMENT_MAIN_AXIS.indexOf(axis.main)<0)axis.main="start";if(ALIGNMENT_CROSS_AXIS.indexOf(axis.cross)<0)axis.cross="stretch";

    I'm not sure what other improvements you want.

  • My change to $scope.layoutAlign simply omits the trailing whitespace if the new none (use default) option is selected in the demo. I suppose a trim might be better:

    return($scope.layoutDemo.mainAxis+' '+$scope.layoutDemo.crossAxis).trim();

    This was done mostly for aesthetic reasons, because the demo would previously display layout-align="space-around ". However, it also helped me test my changes to layout.js, because layout-align="space-around " and layout-align="space-around" were being parsed differently, which is exactly the bug this PR fixes.

@ThomasBurleson

Copy link
Copy Markdown
Contributor

@ngraef - I think my current Flu is confusing me; since I wrote

if(ALIGNMENT_MAIN_AXIS.indexOf(axis.main)<0)axis.main="start";if(ALIGNMENT_CROSS_AXIS.indexOf(axis.cross)<0)axis.cross="stretch";

LOL.

Thank you for the clear and reasonable follow-up.
This looks good and I will merge soon.

@ThomasBurlesonThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: work labels Dec 2, 2015
@ngraef

Copy link
Copy Markdown
ContributorAuthor

@ThomasBurleson - No problem. Thanks for reviewing.

@ngraef
ngraef deleted the fix-layout-align branch September 21, 2016 17:46
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

pr: merge readyThis PR is ready for a caretaker to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

layout-align: Resetting to default when only one argument

2 participants

@ngraef@ThomasBurleson