Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added autoblocking for article header #33

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Added autoblocking for article header #33

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7763b11
Added autoblocking for article header
sergiucoman Oct 19, 2023
07b77e7
Addressed review comments
sergiucoman Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions blocks/article-header/article-header.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
.block .article-byline p {
display: inline;
margin-right: 20px;
font-size: 14px;
font-weight: 400;
}
andreituicu marked this conversation as resolved.
Show resolved Hide resolved


.article-byline a:any-link {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not particularly fond of keeping block specific styles in global CSS, but I think this is not going to become too complex.

font-family: var(--ff-gilroy-bold);
background-image: linear-gradient(to right,var(--link-background-color) 50%,transparent 50%);
background-size: 205% 2px;
background-repeat: no-repeat;
background-position: bottom right;
}

.article-byline p > a:hover {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, we could replace https://github.com/hlxsites/servicenow/blob/main/styles/styles.css#L236 to not duplicate

background-position: bottom left;
}
9 changes: 9 additions & 0 deletions blocks/article-header/article-header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default async function decorate(block) {
const rows = Array.from(block.children);
// title
const titleContainer = rows[0];
titleContainer.classList.add('article-title');
// byLine
const authorContainer = rows[1];
authorContainer.classList.add('article-byline');
}
55 changes: 50 additions & 5 deletions scripts/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
import {
sampleRUM,
buildBlock,
loadHeader,
loadFooter,
decorateBlocks,
decorateButtons,
decorateIcons,
decorateSections,
decorateBlocks,
decorateTemplateAndTheme,
waitForLCP,
getMetadata,
loadBlocks,
loadCSS,
loadFooter,
loadHeader,
sampleRUM,
toClassName,
waitForLCP,
} from './aem.js';
import { span } from './dom-helpers.js';
import { formatDate } from './utils.js';

const LCP_BLOCKS = []; // add your LCP blocks to the list

Expand Down Expand Up @@ -44,6 +47,45 @@ async function loadFonts() {
}
}

/**
* Builds an article header and prepends to main in a new section.
* @param main
*/
function buildArticleHeader(main) {
if (main.querySelector('.article-header')) {
// already got an article header
return;
}

const div = document.createElement('div');
const h1 = main.querySelector('h1');

const author = getMetadata('author');
const authorURL = getMetadata('author-url') || `/authors/${toClassName(author)}`;
const publicationDate = formatDate(getMetadata('publication-date'));

const articleHeaderBlock = buildBlock('article-header', [
[h1],
[`<p><a href="${authorURL}">${author}</a></p>
<p>${publicationDate}</p>`],
]);
div.append(articleHeaderBlock);
main.prepend(div);
Copy link
Collaborator

@andreituicu andreituicu Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
      ]
    ]);
  ),
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice idea.

}

/**
* Returns true if the page is an article based on the template metadata.
* @returns {boolean}
*/
function isArticlePage() {
let blogPage = false;
const template = getMetadata('template');
if (template && template === 'Blog Article') {
blogPage = true;
}
return blogPage;
}

/**
* Builds all synthetic blocks in a container element.
* @param {Element} main The container element
Expand All @@ -52,6 +94,9 @@ async function loadFonts() {
function buildAutoBlocks(main) {
try {
// buildHeroBlock(main);
if (isArticlePage()) {
buildArticleHeader(main);
}
} catch (error) {
// eslint-disable-next-line no-console
console.error('Auto Blocking failed', error);
Expand Down
20 changes: 20 additions & 0 deletions scripts/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
const months = [
'January',
'February',
'March',
'April',
'May',
'June',
'July',
'August',
'September',
'October',
'November',
'December',
];

// 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()}`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}