Images grid block #68
Conversation
|
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
|
Lakshmishri
requested review from
kesiah,
TomaszDziezykNetcentric and
cogniSyb
|
@Lakshmishri could you update the test link? Now it is pointing to the main page without the component you developed. |
Done |
| width: 37px; | ||
| } | ||
|
|
||
| .v2-images-grid-title { |
There was a problem hiding this comment.
Can we use BEM for the classnames? 🙏🏻
There was a problem hiding this comment.
Not sure if the title should be part of the image grid or text block, but in case it is, the designs are using H2 font, not h1.
There was a problem hiding this comment.
Can we use BEM for the classnames? 🙏🏻
Done
There was a problem hiding this comment.
Not sure if the title should be part of the image grid or text block, but in case it is, the designs are using H2 font, not h1.
I have removed it as Tomas mentioned it's part cards block
| } | ||
|
|
||
| .redesign-v2 .v2-images-grid-container .v2-images-grid-wrapper { | ||
| padding: 0 100px; |
There was a problem hiding this comment.
We need to remove the padding: 0 200px from styles and use max-width: var(--wrapper-width) instead. So all the blocks follow the same size.
|
|
|
| background-color: var(--c-secondary-graphite); | ||
| right: 10px; | ||
| text-transform: uppercase; | ||
| font: var(--body-2-font-size)/16px var(--ff-body); |
There was a problem hiding this comment.
Outcome should be font-size: 12px with var(--ff-body-bold) to achieve bold text
| // give format to the first 4 list items | ||
| [...ul.children].forEach((li, idx) => { | ||
| if (idx < 4) { | ||
| const section = createElement('div'); |
There was a problem hiding this comment.
Could we get rid of this <div> that seemingly does nothing?
| text-align: center; | ||
| position: absolute; | ||
| bottom: 10px; | ||
| padding: 9px; |
There was a problem hiding this comment.
Should be padding: 9px 9px 7px;
There was a problem hiding this comment.
I did see that in figma, but rounded to 9px on all sides
|
|
@cogniSyb - can you confirm if this (Images grid carousel#38)is part of MVP . If not we should hide the button for MVP |
| .v2-images-grid__item:nth-child(4) { grid-area: area4; } | ||
|
|
||
| .v2-images-grid__item img { | ||
| height: var(--v2-images-grid-big-height); |
| width: 100%; | ||
| } | ||
|
|
||
| .v2-images-grid__item:nth-child(2) img, .v2-images-grid__item:nth-child(3) img { |
| display: none; | ||
| } | ||
|
|
||
| @media screen and (min-width: 744px) { |
| --v2-images-grid-padding-space: 40px; | ||
| } | ||
|
|
||
| .v2-images-grid__item img { |
|
Fix #24
Test URLs: