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

Fix for Main group #56

Closed
wants to merge 10 commits into from
Closed

Conversation

mghenciu
Copy link
Contributor

@mghenciu mghenciu commented Jul 12, 2023

Summary

This fixes the issue where the main group for all the templates, has some padding on the sides in the editor.
After the fix, there should be no padding in the Editor.

Will affect visual aspect of the product

YES, In the Editor only


Screenshots

Screenshot 2023-07-13 at 09 51 33


Screenshot 2023-07-13 at 09 53 04


Test instructions

  • Install the build
  • open a few templates like 404, front page, index, etc
  • see if the main group has Enabled Inner blocks use content width and/or a transparent bg color set
  • in case the 2 things mentioned above are checked -> the Main Group shouldn't have any padding on the side in the Editor

Closes #54 .

@github-actions
Copy link

github-actions bot commented Jul 12, 2023

Plugin build for 2b8bb1b is ready 🛎️!

Copy link
Contributor

@preda-bogdan preda-bogdan left a comment

Choose a reason for hiding this comment

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

Code changes look good, we can move this to QA for testing.

@rodica-andronache
Copy link

@mghenciu On desktop everything looks ok. I'm wondering if the way it now looks on mobile is ok ( with some sections that don't have any left/right padding ).
Some examples of how it's looking with this PR compared to the current version:
https://vertis.d.pr/i/0rHptW VS https://vertis.d.pr/i/v3CKWw
https://vertis.d.pr/i/XUslY3 VS https://vertis.d.pr/i/rST7tk
Let me know if that's expected or not. Thanks!

@mghenciu
Copy link
Contributor Author

mghenciu commented Jul 13, 2023

Thank you Rodica for checking so promptly.
Yes, you are right - there looks to be some changes on Mobile. I'll review this more, and when ready move again into QA.
Sorry for the inconvenience.


@JohnPixle , apparently using Inner blocks use content width with Neve FSE - there's also a visual issue when previewing Core Styles (attached):
Screenshot 2023-07-13 at 16 25 58

Now, I did some testing with Spectra one (which doesn't uses one main group) and with Frost (which has a main group). And what I noticed in case of Frost, is that they use Inner blocks use content width with all groups, in some they even have values; additionally they use custom padding on the sides - which we don't. Added a video below with Frost:

Screen.Recording.2023-07-13.at.16.37.45.mov

Let me know what you think.
I'll try to test a few more approaches in terms of padding.

@mghenciu
Copy link
Contributor Author

I did some more testing, and I think there may be some issues how we approached it in code, in terms of styling.

Basically what I see, for example in case of the 404 Page - is that we add some Default Padding from code, but at the same time we add a Negative Margin + we add have things differently styles for the wp-site-blocks class, compared to how Frost does it:

Screenshot 2023-07-14 at 11 52 29
Screenshot 2023-07-14 at 11 53 25

@HardeepAsrani HardeepAsrani added the on-hold Not being actively worked by being blocked or not having the capacity to work on it. label Aug 2, 2023
@cristian-ungureanu
Copy link
Contributor

@mghenciu I will close this PR since the fix was provided in this PR: #75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-hold Not being actively worked by being blocked or not having the capacity to work on it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants