[PDP] Fixing whitespace issue for blocks #2107
Conversation
|
Hi @eugenekasimov, i think we did good to remove the extra green margin. I feel like the icon with text block is a bit stuck. Would that be possible to set the same exception we have on description, on icon with text block? Meaning its top and bottom spacing would automatically be 2.5 em? This is highly theory so happy to huddle about it before you start doing anything :) |
|
It looks amazing! Thanks @eugenekasimov |
| @@ -322,7 +322,8 @@ fieldset.product-form__input .form__label { | |||
| } | |||
|
|
|||
| .product__info-container .product-form, | |||
| .product__info-container .product__description { | |||
| .product__info-container .product__description, | |||
| .product__info-container .icon-with-text { | |||
| margin: 2.5rem 0; | |||
There was a problem hiding this comment.
It's not this line affecting it but .accordion are also getting a margin-top: 2.5rem; which is ok when they're after the product description or icon with text but otherwise it might look uneven. Have a look at this example:
Screenshot
Would we maybe want to make that 1.5rem for .accordion on the product page 🤔
I guess this is more of a design decision -> cc: @YoannJailin
There was a problem hiding this comment.
Hey Ludo, I think it's a good idea, thanks. I tested different combinations, seems like this was the only edge case.
I also noticed that the variant-picker block has only margin-bottom: 1.2rem. It's hard to reproduce a scenario to see this because usually this block is somewhere in the middle and there is a margin from other blocks. However, to keep consistency in the code I would change it to 1.5rem for top and bottom? Any thoughts?
There was a problem hiding this comment.
@YoannJailin I think we need your opinion here. Feel free to pair with me on it if you need to see more examples of all of this.
There was a problem hiding this comment.
@ludoboludo @eugenekasimov i think i would not touch accordion's padding in this PR.
- I don't see a merchant using blocks between collapsible row blocks. They were designed to be put together. That usecase seem a bit too specific to be worth solving for now. As this PR was just to fix the new Icon with text block, I propose we don't touch the rows for now so we can this one.
| @@ -967,7 +968,7 @@ a.product__text { | |||
| font-size: 1.6rem; | |||
There was a problem hiding this comment.
Similar thing here. The product-popup-modal__opener is getting a display: inline-block above. Due to it, there is quite a bit of space around it:
Screenshot
If we were to just change the CSS to display: block; for it it would fix it 👍 :
Screenshot, after
Basically it allows for margin collapse to happen and make everything a bit tighter/neater (margin collapse explained).
There was a problem hiding this comment.
Hm 🤔, what if a merchant wants to add two pop-up links next to each other? If we change a display to block they are not going to be in line anymore.
There was a problem hiding this comment.
Oh interesting, never realized that people might do that. Though I wonder how many would and if we have leveraged this in our themes so far 🤔
Another place where we could use @YoannJailin's input
There was a problem hiding this comment.
I agree the spacing is not working. There's definitely extra margins around pop up link compared to the other blocks. Talking with @eugenekasimov and it seems that this would take more time to fix it. I can open another PR we could prioritize for later.
* Remove padding-top for icon-with-text. Add default height for share-button. * Add margin for icon-with-text block * Change height to min-height and width to min-width.
PR Summary:
Why are these changes introduced?
Fixes #2064 .
What approach did you take?
.product__info-container .icon-with-textOther considerations
Visual impact on existing themes
Before
After
Testing steps/scenarios
Demo links
Checklist