Skip to content
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

remove-full-width-image-mobile-margin-1585: remove full width image block margins on mobile #1713

Merged

Conversation

9ete
Copy link
Contributor

@9ete 9ete commented Jun 21, 2021

Moved styles targeting entry content full width images from applying only on 1024+ to apply on all viewport sizes. This fixes the issue of image blocks marked as full width not going full width on mobile.

Steps to test the fix (originally posted here: #1585 (comment)):

  • Add an Image block to a new page, set it to full width;
  • Make sure the page's template is also set to Full Width, and save/publish the changes;
  • Open the created page on a browser with at least 1064px width. Verify that the image is full width.
  • Switch to browser responsive mode and reduce the screen size to 1063px width or less. Verify that there are no margins on both sides.

Before Fix
Screen Shot 2021-06-23 at 11 23 54 AM
Screen Shot 2021-06-23 at 11 23 35 AM

After Fix
Screen Shot 2021-06-23 at 11 21 45 AM
Screen Shot 2021-06-23 at 11 21 32 AM

@9ete 9ete requested a review from a team as a code owner June 21, 2021 22:17
@9ete 9ete requested review from opr and removed request for a team June 21, 2021 22:17
@9ete
Copy link
Contributor Author

9ete commented Jun 21, 2021

@opr for issue: #1585

@opr opr added category: styles Issues related to styling priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. type: bug The issue is a confirmed bug. labels Jun 22, 2021
@opr
Copy link
Contributor

opr commented Jun 23, 2021

Hi @hawkeye126 thanks for making this PR! It would be really useful if you could edit the PR description to include a summary of what you changed and to add some testing steps. If you ping me back when you've done this I'll be able to take a look.

@9ete
Copy link
Contributor Author

9ete commented Jun 23, 2021

@opr Done, thanks for the feedback and reviewing! Please let me know if I can provide any further detail.

Copy link
Contributor

@opr opr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hawkeye126 - I've tested this and it looks to be working well! 👏🏼 Congratulations on your first PR to Storefront! 🥳

I also ran npm run lint:css on my local machine and it passes. The job fails in GitHub due to permission errors I believe. More information on why the linting job failed: actions/first-interaction#10

@opr opr merged commit b0d05ba into woocommerce:trunk Jun 25, 2021
@opr opr added this to the 3.8.0 milestone Jun 25, 2021
@vanderwijk
Copy link
Contributor

There seems to be an issue with this on my site. After updating to 3.8.0 my full width cover block is no longer spanning the full width of the page. This is because the width is set to 100% instead of auto.

screenshot

Adding auto width fixes this for me:

.storefront-align-wide.page-template-template-fullwidth-php & .alignfull, .storefront-align-wide.storefront-full-width-content & .alignfull { margin-left: calc(50% - 50vw); margin-right: calc(50% - 50vw); width: auto; }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: styles Issues related to styling priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants