-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
Ready to make a difference block #46
Conversation
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
blocks/career-apply/career-apply.js
Outdated
const linkedin = document.createElement('a'); | ||
linkedin.innerText = placeholders['career-apply-linkedin']; | ||
linkedin.classList.add('button', 'primary', 'linkedin'); | ||
linkedin.href = '#'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the code to pick these up from the placeholders.xslx - that way they don't have to be duplicated on every career testimonial page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdmcraft do you think this is an acceptable approach?
@bosschaert An impressive title, I thought you might create a crazy block at first glance. ^_^ |
Hi @JiangLong2019 yes I see that they are nearly identical. The one you are referring to has a video on the background. Currently this block doesn't support that, but I think it could be easily extended to support it, so yes let's reuse it there too. |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
🔸 1 visual difference detected
The diff images are attached in the artifact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM added few comments. @sdmcraft it would be good if you can once check the logic for picking href from placeholders.json
blocks/career-apply/career-apply.css
Outdated
.career-apply { | ||
color: var(--white); | ||
text-align: left; | ||
max-width: unset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max-width is not needed here
Looks right to me as authors just have to manage it at 1 place. The other option could be to maintain it in a fragment. |
🔸 1 visual difference detected
The diff images are attached in the artifact |
|
Fixes #16
Test URLs:
Before: https://main--sunstar--hlxsites.hlx.page/career/yuya-yoshisue
After: https://issue-16--sunstar--hlxsites.hlx.page/career/yuya-yoshisue
If you are adding a new block or a variation to an existing block, has it been added in the block-inventory ?
Will add the block to the inventory when all three career testimonial blocks are done (this is the second one).