Conversation
|
helms-charity
left a comment
helms-charity
left a comment
There was a problem hiding this comment.
In addition to my inline comments, can you also fix this bug:
- at accordion.css line 18, remove "width: 100%" since it is not needed for either type of accordion. This will fix the first down arrow item from being narrower than all the rest.
The biggest problem is content being cut off in mobile.
aemedge/blocks/tabs/tabs.css
Outdated
| /* Additional spacing for larger screens */ | ||
| @media (width >= 992px) { | ||
| .tabs.big .accordion-wrapper { | ||
| padding: 0 4rem; |
There was a problem hiding this comment.
Remove this at line 345 to make the spacing match the original better.
aemedge/blocks/tabs/tabs.css
Outdated
| color: #454550; | ||
| font-weight: bold; | ||
| text-transform: uppercase; | ||
| font-size: 20px; |
There was a problem hiding this comment.
Live site has smaller font in mobile. Change 317 to
font-size: 16px;
Then add font-size: 20px for width > 768px.
aemedge/blocks/tabs/tabs.css
Outdated
| } | ||
|
|
||
| /* "big" variant */ | ||
| .tabs.big .tabs-list button { |
There was a problem hiding this comment.
You can remove lines 277-280 because the font and spacing in your version compared to the live.
aemedge/blocks/tabs/tabs.css
Outdated
|
|
||
| /* stylelint-disable-next-line no-descending-specificity */ | ||
| .tabs.big .tabs-list button p { | ||
| font-size: 1.25rem; |
There was a problem hiding this comment.
remove line 284, to keep the original font size here.
aemedge/blocks/tabs/tabs.css
Outdated
| order: 2; /* Move arrow to end */ | ||
| margin: 4px 0 0 2rem; /* Adjust margins for right alignment */ | ||
| position: absolute; | ||
| right: 0; |
There was a problem hiding this comment.
Can you also add background: url here to use the larger "V" style icon like the original? (I see we have the white version of that in the mobile footer).
aemedge/blocks/tabs/tabs.css
Outdated
| .tabs.big .accordion-wrapper { | ||
| max-width: 1200px; | ||
| margin: 2rem auto; | ||
| padding: 0 2rem; |
There was a problem hiding this comment.
Change lines 288-292 to:
.tabs.big .accordion-wrapper {
max-width: 1130px;
margin: 1rem auto;
padding: 0 1rem;
}
to better match the live version
aemedge/blocks/tabs/tabs.css
Outdated
|
|
||
| .tabs.big .accordion .details .accordion-item-body { | ||
| font-size: 1.125rem; | ||
| padding-left: 4rem; |
There was a problem hiding this comment.
Replace line 320-324 with the below to better match the live site (don't need font-size here).
.tabs.big .accordion .details .accordion-item-body {
padding-left: 0;
line-height: 1.5;
}
aemedge/blocks/tabs/tabs.css
Outdated
| height: 128px; | ||
| object-fit: contain; | ||
| display: block; | ||
| margin: 1rem 0; |
There was a problem hiding this comment.
Remove margin at line 355
| /* stylelint-disable-next-line no-descending-specificity */ | ||
| .tabs.big .accordion .details .accordion-item-body picture img { | ||
| width: 128px; | ||
| height: 128px; |
There was a problem hiding this comment.
I don't know how best to fix this, but because there is a JS calculated max-height on the .accordion-item-body, having the height: 128px on the image is pushing the bottom of the text out of the visible area.
There was a problem hiding this comment.
This happens when we responsively change width of screen. but not if directly opened on mobile device.
There was a problem hiding this comment.
Ok, thanks -- I think that is acceptable.
helms-charity
left a comment
helms-charity
left a comment
There was a problem hiding this comment.
Just reverse the arrow direction and you can merge, everything else looks good.
aemedge/blocks/tabs/tabs.css
Outdated
| position: absolute; | ||
| right: 9px; | ||
| top: 50%; | ||
| transform: translateY(-50%) rotate(-135deg); |
There was a problem hiding this comment.
Make this rotate(45deg)
aemedge/blocks/tabs/tabs.css
Outdated
|
|
||
| /* Rotate arrow when accordion is open */ | ||
| .tabs.big .accordion .details.open .summary::before { | ||
| transform: translateY(-50%) rotate(45deg); |
There was a problem hiding this comment.
Make this rotate(-135deg) and then it will match the position of the mobile footer accordion arrow positions.
| /* stylelint-disable-next-line no-descending-specificity */ | ||
| .tabs.big .accordion .details .accordion-item-body picture img { | ||
| width: 128px; | ||
| height: 128px; |
There was a problem hiding this comment.
Ok, thanks -- I think that is acceptable.
2 participants
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 #533
Test URLs: