✨ Adding themes for new outlink CTA #33702
Conversation
|
Hey @gmajoulet, @newmuis, @Enriqe! These files were changed: Hey @ampproject/wg-caching! These files were changed: |
|
Transition state for the new outlink UI will be built with: #33812 |
|
JS/CSS looks good, left comments on the DD for the API but let's continue that offline (and could be changed later since this is behind an experiment) |
|
This pull request introduces 3 alerts when merging 8f10a84 into 4d9ef0e - view on LGTM.com new alerts:
|
| } else { | ||
| const linkImage = htmlFor( | ||
| attachmentEl | ||
| )`<svg class="i-amphtml-story-page-open-attachment-link-icon" xmlns="http://www.w3.org/2000/svg" width="24" height="24"> |
There was a problem hiding this comment.
Can the SVG be living in the CSS instead, and added through a class?
There was a problem hiding this comment.
The CSS needs to change the color based on the accent color (the same color as the text), and CSS background-image can't access CSS vars or be changed through JS. The simplest workaround is to have the SVG in the HTML allowing the use of the CSS vars, unless you had another way fo doing this (like tinting background-images or other sorcery)
| let themeAttribute = attachmentEl.getAttribute('theme'); | ||
| if (themeAttribute) { | ||
| themeAttribute = AttachmentTheme[themeAttribute.toUpperCase()]; | ||
| } |
There was a problem hiding this comment.
| let themeAttribute = attachmentEl.getAttribute('theme'); | |
| if (themeAttribute) { | |
| themeAttribute = AttachmentTheme[themeAttribute.toUpperCase()]; | |
| } | |
| let themeAttribute = attachmentEl.getAttribute('theme').toLowerCase(); |
Simplifying a bit, this should give you the same value
There was a problem hiding this comment.
This doesn't work bc theme isn't always set, so there needs to be a check. I went with:
let themeAttribute = attachmentEl.getAttribute('theme');
if (themeAttribute) {
themeAttribute = themeAttribute.toLowerCase();
}
* outlinke cta * restoring arrow * adding default and customizable image * adding default and customizable image * adding example pages * adding example pages - nit * refactor adding image * adding instructions * using flexbox instead of table layouts * fixes * removing click target * lint * htmlrefs * conditional fix * added unit test * added tests * CSS classes * test nits * createElementWithAttributes * only one additional class * removes default image class * added dark theme and custom theme * trying to do contrast calculations * added contrast calculations * added validator tests * merge conflicts * merge conflicts * merge conflicts * changed arrow color, added new SVG customization function * trying to customize SVG * trying to customize SVG * added custom SVG method * custom SVG * css vars * cleanup * adding unit test * adding border around custom image + nits * moved rgb logic into open-pg-attachment * removed border * removed added element * css nits * enum classes * added setCustomThemeStyles(win, attachmentEl, openAttachmentEl) method * nits * theme enum fix * fix unit test
The following themes are now available for the outlink CTA button:
Firebase demo: https://test-project-01-ead4f.web.app/examples/amp-story/attachment.html
Fixes #32716
Fixes #32718