Layout calculation refactored #1094
Conversation
|
|
||
| // Layout specified directly. | ||
| if (layoutAttr) { | ||
| layout = parseLayout(layoutAttr.trim()); |
There was a problem hiding this comment.
Quite sure the trim() can go, since there's no currently considered valid document that would need it.
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 66fb039 (Please note that this is a fully automated comment.) |
| if (!layout) { | ||
| if (widthAttr || heightAttr) { | ||
| if (!widthAttr || widthAttr == 'auto') { | ||
| layout = Layout.FIXED_HEIGHT; |
There was a problem hiding this comment.
Layout here is dependent on a width / height value that may have been previously set depending on a layout value, up in l. 148 etc. This circularity makes it really hard to understand what is going on, and it's unnecessary, since determineLayout in this PR gets by without it:
https://github.com/ampproject/amphtml/pull/1073/files
There was a problem hiding this comment.
I disagree that there's circularity. This order is correct to really understand how this works: width and height have to be defaulted first, before layout can be. We have to "think like AMP" here.
There was a problem hiding this comment.
If amp wants to default width and height first, that's ok. But then it's not allowed to use layout in doing so - see line 144, where it's checking layout to particular values. That's the circularity I mean here...
Note that the circularity gives rise to rather funky interpretations based on the data returned by getNaturalDimensions. For instance, assume it returns {width: 'auto', height: '1px'} (yes I know it doesn't), and the given width/height attributes are not set. Now it'll affect l. 156 and set layout to FIXED_HEIGHT. Resolving the circularity leads to a stricter behavior, which is desirable in my opinion, because it's easier to understand locally. Specifically, the choice of layout no longer depends on the return value of getNaturalDimensions - it's sufficient to know that an element hasNaturalDimensions, and the actual dimensions are only used to set width and height.
There was a problem hiding this comment.
There can't be width:auto - that wouldn't be the natural dimension. You can simply default them to '1' if it's convenient for you code - you don't need to resolve them.
|
@ampsauce retry |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: cca8d2a (Please note that this is a fully automated comment.) |
|
@powdercloud PTAL. Input and effective layout values have been separated. |
|
|
||
| // Input layout attributes. | ||
| const inputLayout = layoutAttr ? parseLayout(layoutAttr) : null; | ||
| assert(inputLayout || !layoutAttr, 'Unknown layout: %s', layoutAttr); |
There was a problem hiding this comment.
Is it cool to switch to assert? Sorry I don't know the practical difference in Javascript between that and exceptions ... I guess I wonder whether exceptions might be caught by something vs. assertions give up on the entire document, but I'm not familiar with assert in Javascript. Could be a non-issue, sorry.
There was a problem hiding this comment.
Should the condition be something like this?
assert(inputLayout !== undefined, 'Unknown layout...);
There was a problem hiding this comment.
Yes. Asserts are fine. In this case I preferred it because the expressions look a lot cleaner.
The inputLayout !== undefined is equivalent. I'll switch to it.
|
Thanks much @dvoytenko. I looked through this carefully and will try to do the equivalent thing for the validator. Looks like there is some test in travis (amp-audio related?) that's still unhappy. |
|
@powdercloud PTAL |
|
@dvoytenko Thanks a lot, LGTM. |
Layout calculation refactored
The goal is to create a clear sequence of defaults and calculations and separate it from UI.