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

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

Closed
wants to merge 1 commit into from

Conversation

ngraef
Copy link
Contributor

@ngraef ngraef commented Dec 1, 2015

  • 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

fixes angular#5996
@ThomasBurleson
Copy link
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.

@ThomasBurleson ThomasBurleson added this to the 1.0-rc6 milestone Dec 2, 2015
@ThomasBurleson ThomasBurleson self-assigned this Dec 2, 2015
@ngraef
Copy link
Contributor Author

ngraef commented Dec 2, 2015

@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
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
Contributor Author

ngraef commented Dec 2, 2015

@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
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.

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

ngraef commented Dec 2, 2015

@ThomasBurleson - No problem. Thanks for reviewing.

@ngraef ngraef deleted the fix-layout-align branch September 21, 2016 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This 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