✨ Implements variable substitution macro for html element opacity. #16479
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
src/inabox/inabox-viewport.js
Outdated
| @@ -174,6 +174,27 @@ export class ViewportBindingInabox { | |||
| }); | |||
| } | |||
|
|
|||
| /** @override */ | |||
| getElementOpacity(el) { | |||
There was a problem hiding this comment.
To not blow the size of AMP runtime, we'd like this to be moved to amp-analytics extension.
So no change needed to resource.js, viewport-*.js.
You can probably introduce a new file opacity.js under amp-analytics/0.1/ and have all the logic there.
| * @param {!Element|null} el | ||
| * @return {number} minimum opacity value | ||
| */ | ||
| export function getOpacity(el) { |
There was a problem hiding this comment.
nitpick: pls put public functions at the top
| * @param {!Element|null} el | ||
| * @return {number} minimum opacity value | ||
| */ | ||
| export function getOpacity(el) { |
There was a problem hiding this comment.
should we multiply all the opacity values together?
it min opacity is really what you want, let's rename the method.
|
|
||
|
|
||
| /** | ||
| * Returns the min opacity found amongst the element and its parents |
|
@lannka thank you for the review! I have made the changes you requested. |
|
@lannka should we make the PR to expose this as a macro or will that be handled on your end? |
|
@joel-regus thinking twice, let's not worry about that, as it's already exposed in the |
|
@lannka is this ready to merge? |
|
Pinging @lannka for next steps here. |
|
@lannka is there any guidance you can provide in regards to these failing unit tests? We are not sure as to how to proceed. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
joel-regus
force-pushed
the
moat_opacity_update
branch
from
1f6d233
to
0e6da80
Compare
|
CLAs look good, thanks! |
…mpproject#16479) * Create variable substitution macro that returns the opacity value of an element. * fix typo * Add Unit Tests * Update unit tests * rename variable to prevent linter errors * move opacity to a new file * removed opacity code from src * remove dead code * remove dead code * Added license to opacity * Added license to opacity test * Moved public function to top of file * Change getOpacity to getMinOpacity * Change getRootOpacity to getRootMinOpacity * Sort imports alphabetically * change parents to ancestors in comment
This change creates a new variable substitution macro called "opacity" that returns a selected html element's level of transparency, the values range from 0 to 1.
This change also takes into account the element's location in the DOM tree. It is possible for an element to have its opacity set to 1 (fully visible), yet its parent can be set to 0 (fully invisible). The goal is to return the opacity value that is visible by a human. If we are unable to return an element's opacity then the default value returned is 1.