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
Adjust spacing in/between elements on the Cart and Checkout block pages #44160
Adjust spacing in/between elements on the Cart and Checkout block pages #44160
Conversation
Test Results SummaryCommit SHA: 5017116
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Hi @wavvves, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Thank you for the suggestions, @senadir. I'm moving the PR to draft to make the suggested changes in Slack: p1707223474247879-slack-C8X6Q7XQU |
… from email field
@senadir @nikkivias I have updated the line height for the set description and fixed the margin-top of the email field. Here is how it looks now: I feel it looks good now. I'd love to hear your thoughts about it. |
Hi team, there are some of the things I noticed. Regarding the item separators in the order summary, there should be a bigger gap after the border, but its not accounting for the item quantity so it looks tight right now. However, I am thinking we remove the borders all together for a cleaner look. Also if you notice they are shorter and not aligned with the other borders. |
@nikkivias Thank you for reviewing the changes. I'd appreciate some input for the following: The below items are as per the design. I'd be happy to update it. Please let me know how much gap we should leave for them: Cart Totals
Order Summary Gap
The images needs to be moved down:
Product SeparatorWe discussed this earlier in Figma: 2JyXNimI4oGeJZhgQNZE9V-fi-819:26489#684299023 about keeping the separator. I won't mind removing it; please let me know. Should we keep it at full width or remove it? Site headerPlease ignore it. This is the site header and should be controlled by the Merchant while designing their site. It will be the same throughout the site. Payment option gap
The problem is with the multiple packages option. It should have a 16px gap instead of 8 px. I will fix that in the next commit: Sub text space for the payment methodSure, I'll update it in the next commit. Additional borderI will fix it in the next commit. Line-height in cart line itemsChanging the line height to these numbers will shrink the space between them. Are we sure we need to change the line height for them? Screen.Recording.2024-02-10.at.12.50.24.AM.movScreen.Recording.2024-02-10.at.12.48.40.AM.movI'll wait for your reply before making further changes; please take your time and let me know. |
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.
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.
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.
@nielslange those seems to be coming from the theme and applied to the page layout, and not from the block? editing those directly would impact the theme control negatively. |
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.
If that's the case, then I'm unable to test step 3. of the testing instructions, which states:
On my end, I simply see different spaces than the ones mentioned in the design. |
Thank you so much for the feedback, @senadir and @nielslange. I'll take a look and get back to you on this. Ideally, we should not have these inconsistencies. |
@nielslange It was happening because I was using em conversion. I replaced it with normal px, and it should be fine now. I'd appreciate it if you could review the PR again. |
I agree with @senadir. Since this is outside the block, we should not manipulate the theme layout. It should be controlled by the merchant. Screen.Recording.2024-02-16.at.8.13.50.PM.mov |
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.
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.
Approving this, thank you for the patience @tarunvijwani, mobile looks way better now :)
Thank you for the suggestions and feedback, @wavvves! 🎉 |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #42108.
With this PR, we're adjusting spacing in/between elements on the Cart and Checkout block pages as per the design,
How to test the changes in this Pull Request:
Design
Figma link: 2JyXNimI4oGeJZhgQNZE9V-fi-819_26489
Changelog entry
Significance
Type
Message
Adjust spacing in/between elements on the Cart and Checkout block pages.