feat(content-band-checkmarks): Content band checkmarks block changes #300
Conversation
nimithshetty17
requested review from
proeung,
keith-kaplan,
sachinmesh,
sharathmrft and
sahmad-merative
|
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
|
There was a problem hiding this comment.
@nimithshetty17
Responsiveness and Background color is fine on different devices but 'Video player' functionality needs to be configured and needs to be verified to play the video as per the design.
@proeung @sachinmesh
|
In desktop, the width of the right side list rounded rectangles should be 360px. Rest is good to go. |
proeung
added
the
Needs design QA
|
|
Marking this PR as "needs fixing", since video button functionality still needs to be implemented. |
|
@proeung - Had a word with Saad and once his PR for video player is reviewed and merged, I will use the same functionality for the video modal in content band checkmarks as well. |
|
@nimithshetty17 Okay, sounds good. Let's keep this PR as on-hold until this PR (#307) has been approved and merged. |
proeung
added
On-Hold
|
|
@sachinmesh - The width fix is added and can be reviewed. |
|
Ya, checked @nimithshetty17 , now the width on the lists is fixed. Thank you. |
proeung
changed the title
|
|
|
@nimithshetty17 This PR can be moved forward since this one (#307) has been merged. |
There was a problem hiding this comment.
Good first pass! However, I added some code suggestions so that we can keep to using CSS variables whenever we can.
Also, let's add a content-band-checkmarks.js file and add a class to the <p> tag that we're using for the eyebrow (see - https://github.com/hlxsites/merative2/blob/main/blocks/content/content.js#L15-L25C8)
Your solution for targeting the .content-band-checkmarks > div > div:first-child > p:first-child works great if the first <p> is an eyebrow, but there might be a scenario where the first <p> in the DOM is the short description because the eyebrow content is not used. Let's try to adjust the return of the markup as best as possible.
| @media (min-width: 1200px) { | ||
| .content-band-checkmarks > div { | ||
| display: flex; | ||
| gap: 120px; |
There was a problem hiding this comment.
| gap: 120px; | |
| gap: var(--gap-120); |
see - https://github.com/hlxsites/merative2/blob/main/styles/styles.css#L198C3-L198C12
| grid-template-columns: repeat(2, 1fr); | ||
| grid-template-rows: repeat(3, 1fr); | ||
| grid-auto-flow: column; | ||
| column-gap: 24px; |
There was a problem hiding this comment.
| column-gap: 24px; | |
| column-gap: var(--gap-24); |
Let's move this into CSS variable (--gap-24: 24px;). https://github.com/hlxsites/merative2/blob/main/styles/styles.css#L196
| } | ||
|
|
||
| .content-band-checkmarks > div > div:nth-child(2) > ul>li::before { | ||
| margin-right: 24px; |
There was a problem hiding this comment.
| margin-right: 24px; | |
| margin-right: var(--spacer-element-07); |
| font-weight: var(--font-weight-medium); | ||
| line-height: var(--line-height-160); | ||
| letter-spacing: var(--letter-spacing-001-em); | ||
| margin: 0 0 16px; |
There was a problem hiding this comment.
| margin: 0 0 16px; | |
| margin: 0 0 var(--spacer-element-05); |
| line-height: var(--line-height-160); | ||
| letter-spacing: var(--letter-spacing-001-em); | ||
| margin: 0 0 16px; | ||
| padding: 0 24px; |
There was a problem hiding this comment.
| padding: 0 24px; | |
| padding: 0 var(--spacer-element-07); |
| padding: 0; | ||
| } | ||
|
|
||
| .content-band-checkmarks > div > div:nth-child(2) > ul>li { |
There was a problem hiding this comment.
| .content-band-checkmarks > div > div:nth-child(2) > ul>li { | |
| .content-band-checkmarks > div > div:nth-child(2) > ul > li { |
| margin: 0 0 var(--spacer-layout-04); | ||
| } | ||
|
|
||
| .content-band-checkmarks > div > div:first-child > .button-container>strong>a { |
There was a problem hiding this comment.
| .content-band-checkmarks > div > div:first-child > .button-container>strong>a { | |
| .content-band-checkmarks > div > div:first-child > .button-container > strong > a { |
| } | ||
|
|
||
| main > .section.section-header.list-section.bg-neutral-bone > .content-band-checkmarks-wrapper > .content-band-checkmarks > div > div:nth-child(2) > ul>li { | ||
| background-color: white; |
There was a problem hiding this comment.
| background-color: white; | |
| background-color: var(--neutral-white); |
| align-items: center; | ||
| } | ||
|
|
||
| main > .section.section-header.list-section.bg-neutral-bone > .content-band-checkmarks-wrapper > .content-band-checkmarks > div > div:nth-child(2) > ul>li { |
There was a problem hiding this comment.
| main > .section.section-header.list-section.bg-neutral-bone > .content-band-checkmarks-wrapper > .content-band-checkmarks > div > div:nth-child(2) > ul>li { | |
| main > .section.section-header.list-section.bg-neutral-bone > .content-band-checkmarks-wrapper > .content-band-checkmarks > div > div:nth-child(2) > ul > li { |
| } | ||
|
|
||
| .content-band-checkmarks > div > div:first-child > p:not(:first-child):not(.button-container) { | ||
| max-width: 517px; |
There was a problem hiding this comment.
Let's keep the max-width value to 516px so that we don't have just one pixel off.
.content-band-checkmarks > div > div:first-child > h2,
.content-band-checkmarks > div > div:first-child > p:not(:first-child):not(.button-container) {
max-width: 516px;
}
|
|
|
|
|
@sachinmesh @proeung @sharathmrft - The video player functionality has been implemented on section content band checkmarks. |
There was a problem hiding this comment.
@nimithshetty17 Video player functionality has been Tested across the browsers it is working as expected
|
|
|
proeung
added
Ready to merge
Issue
Fixes #MERATIVE-780
Description
This ticket is part of the phase 3 effort, which is to migrate all of the remaining components/patterns in AEM over to Franklin. for Section > Section Content band checkmarks.
Note: part of this work is to clean up the naming in Franklin to ensure it aligns with our Design Systems.
New
Design Specs
https://www.figma.com/file/vw24IPXXt4vCh9BNumka9A/Web--Merative-Digital-Design-System-2.0?type=design&node-id=1459-12919&mode=design&t=7UVLWpIfJsh1PJaS-0
Test URLs
https://section-content-band-checkmarks--merative2--nimithshetty17.hlx.page/block-library/sections/section-content-band-checkmarks
Testing Instruction
To test the content band checkmarks and its responsiveness with respect to the design across all devices.