Added autoblocking for article header #33
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
|
|
|
scripts/scripts.js
Outdated
| <p>${publicationDate}</p>`], | ||
| ]); | ||
| div.append(articleHeaderBlock); | ||
| main.prepend(div); |
There was a problem hiding this comment.
If I may make a suggestion here, I wrote a small dom helper script in an attempt to make the JS look as close as possible to the resulting dom. Using it would look like, if you like it:
main.prepend(
div( //section
buildBlock('article-header', [
[
main.querySelector('h1')
],
[
p(a{ href: authorURL}, author}),
p(publicationDate),
]
]);
),
)
scripts/utils.js
Outdated
| // eslint-disable-next-line import/prefer-default-export | ||
| export function formatDate(date) { | ||
| const d = new Date(date); | ||
| return `${months[d.getMonth()]} ${d.getDate()}, ${d.getFullYear()}`; |
There was a problem hiding this comment.
There is a built in Javascript functions for this:
d.toLocaleDateString('en-US', {
month: 'long',
day: '2-digit',
year: 'numeric',
})and we'll need to see how to correctly display it in the geosites too.
I would maybe also not create a new JS file just for this function. For the moment, until we feel like it grew too much, we can add them to scripts.js to avoid an extra request.
| } | ||
|
|
||
|
|
||
| .article-byline a:any-link { |
There was a problem hiding this comment.
We could replace this selector here: https://github.com/hlxsites/servicenow/blob/main/styles/styles.css#L227 to not duplicate the link CSS.
We can keep the font family here and cleanup in styles.css https://github.com/hlxsites/servicenow/blob/main/styles/styles.css#L240-L242
font-family: var(--ff-gilroy-bold);
There was a problem hiding this comment.
I am not particularly fond of keeping block specific styles in global CSS, but I think this is not going to become too complex.
| background-position: bottom right; | ||
| } | ||
|
|
||
| .article-byline p > a:hover { |
There was a problem hiding this comment.
same here, we could replace https://github.com/hlxsites/servicenow/blob/main/styles/styles.css#L236 to not duplicate
|
Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):
Fix #29
Test URLs: