Small clarification for how some natural dimensions get calculated. #1053
Conversation
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 6d255d84704b2c13b6269fd8c77a18d6267475c9 (Please note that this is a fully automated comment.) |
| * Set or cached browser natural dimensions for elements. The tagname | ||
| * initialized here will return true `hasNaturalDimensions`, even if yet to be | ||
| * calculated. Exported for testing. | ||
| * Set of cached browser natural dimensions for elements. The tagname |
There was a problem hiding this comment.
Both descriptions do not talk to the API. It should read simply "Returns the natural dimensions of an element with the specified name. This operation can only be completed for an element whitelisted by hasNaturalDimensions".
Or something like this. I'd certainly appreciate if you'd update this.
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: ba25107bdf737f99fbff306eafb59d04d6dc8edd (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 6bfa1e511e1bd89bf39cdb680aca9072b00dc783 (Please note that this is a fully automated comment.) |
|
@dvoytenko Thank you, I put the assert in and massaged the prose some more. Please take another look. |
| * `hasNaturalDimensions` checks for membership in this set. | ||
| * `getNaturalDimensions` determines the dimensions for an element in the | ||
| * set and caches it. | ||
| * Exported for testing. |
There was a problem hiding this comment.
No need for "Exported for testing". It already says it below.
|
LGTM with one small comment. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 84c2bf97345f74ed2df63e3af43e220dfa7df0d8 (Please note that this is a fully automated comment.) |
Use an assertion to document that getNaturalDimensions can't get called on an element that's not whitelisted by hasNaturalDimensions.
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 9ab4ce2 (Please note that this is a fully automated comment.) |
|
@dvoytenko Removed "Exported for testing" and updated the commit message. |
I like an explicit === null check better because it makes sure
this only triggers when there's an existing entry in the
table with a null value. I think in practice, while the
getNaturalDimensions function would fill in values for tags that
don't have an entry (say, amp-video), this doesn't happen
because it only gets called if hasNaturalDimensions returns false.
That's my reading anyways. No offense taken if I'm wrong or this
doesn't make things clearer.