Newsletter subscribe page #210
Conversation
|
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
|
| .newsletter-subscribe.block .subscribe-form .title-wrapper [slot="tos-top"] { | ||
| color: #1f2022; | ||
| text-align: center; | ||
| font-family: "Times New Roman", Times, serif; |
There was a problem hiding this comment.
Can you move that font into a CSS variable (like you did with --circle-svg) ?
There was a problem hiding this comment.
I created a times variable.
It might also be worth mentioning, on the original golf site, they probably didn't intend to use times now roman, but something went wrong with the font definition.
I opted to copy it as it appears though since I would assume if it was a problem GD would have fixed it by now.
| } | ||
|
|
||
| .newsletter-subscribe.block .subscribe-form .controls-wrapper .email-input.valid { | ||
| background-image: url("https://cdns1.gigya.com/gs/i//screenSet/checkmarkValid.png"); |
There was a problem hiding this comment.
Instead of relying on cdns1.gigya.com, you could transform the image into a base64 string so at least it will always work independent of cdns1.gigya.com.
Or you could publish it so it uses frnaklin but maybe that's a bit of a hassle for such a small image. Up to you.
There was a problem hiding this comment.
now uses base64
| border: none; | ||
| color: white; | ||
| text-transform: uppercase; | ||
| background-color: #ed3e49; |
There was a problem hiding this comment.
I created #214 to clean up CSS colors.
scripts/scripts.js
Outdated
| return false; | ||
| }); | ||
|
|
||
| return foundSpreadsheet ? foundSpreadsheet.replaceAll(' ', '-') : false; |
There was a problem hiding this comment.
You might want to reuse bits of https://github.com/adobe/helix-onedrive-support/blob/06a32ca7486fad83d38ed4e2b8507b53822c7191/src/utils.js#L36-L43 to support edge cases like multiple spaces.
There was a problem hiding this comment.
using toClassName instead. It gets the job done perfectly
scripts/scripts.js
Outdated
| @@ -192,7 +193,16 @@ export function addPhotoCredit(pictures) { | |||
| * @return {string|null} | |||
| */ | |||
| function findTemplate(className) { | |||
| return Object.values(ARTICLE_TEMPLATES).find((template) => template === className); | |||
| return Object.values(ARTICLE_TEMPLATES).find((template) => { | |||
| if (!className.startsWith(template)) { | |||
There was a problem hiding this comment.
Instead of startsWith I'd prefer to keep exact matching to support templates that start the same eg Gallery and GalleryListicle. You can simply keep the first part of the string before the "(" eg className.split('(')[0].trim() to compare with the template.
There was a problem hiding this comment.
The classname that is checked for doesn't have brackets in it, any special characters are replaced with -. In this case the class that is checked looks like this: newsletter-subscribe-email-subscribe-list-custom-data.
I could change buildTemplate function to use the metadata instead of the body classlist if you prefer
There was a problem hiding this comment.
I could change buildTemplate function to use the metadata instead of the body classlist if you prefer
Yes I think that would make sense
| // Adding base template class to body because any queries added to the | ||
| // template metadata entry are combine which will cause CSS issues. | ||
| // example: "Template (sheet query)" has a class of: "template-sheet-query" | ||
| document.body.classList.add(template); |
There was a problem hiding this comment.
I think this should not be done in findTemplate since the function as its name implies should only find the template, not have side effects like adding a class. Maybe you can do that rather after the function call if a template is found ?
There was a problem hiding this comment.
this was moved out of the findTemplate function.
The findTemplate function no longer exists as it is not used with the new version of buildTemplate
|
@DriftingSands in the google document when you specify queries, can you pls use spaces instead of dashes as it's more natural and common to use spaces to separate words. |
added times font variable. replaced hex color value with css variable.
|
|
* remove font-times
|
Adds newsletter subscribe page, with page functionality. Also adds window.store.query functionality to page templates
Fix #192
Test URLs: