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

Limited Global Styles: Fix modal layout #76133

Merged
merged 5 commits into from
Apr 28, 2023
Merged

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Apr 24, 2023

Fixes #76089

Proposed Changes

As of Gutenberg 15.2, the Modal component no longer renders the passed React nodes as direct children of .components-modal__content, effectively breaking the flex layout used by our Limited Global Styles gating modal.

To fix that, we now use a wrapper with a custom class so we can build a flex layout without relying on how Gutenberg renders the Modal component.

Before After
image Screenshot 2023-04-24 at 13 58 47

Testing Instructions

  • Apply these changes to your sandbox
  • Create a new site and sandbox it
  • Go to the site editor and open the styles sidebar
  • Make sure the modal has the proper layout

@mmtr mmtr added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 24, 2023
@mmtr mmtr requested a review from a team April 24, 2023 12:05
@mmtr mmtr self-assigned this Apr 24, 2023
@github-actions
Copy link

github-actions bot commented Apr 24, 2023

@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit fix/limit-global-styles-modal on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@BogdanUngureanu
Copy link
Contributor

Works as expected on desktop. There're some small issues on mobile resolutions (probably there were happening before too).

Screenshot 2023-04-24 at 17 27 27
Screenshot 2023-04-24 at 17 27 56

Notice the scrollbar and the line that cuts the close button.

@dsas
Copy link
Contributor

dsas commented Apr 25, 2023

On the vertical phone screenshot should we display the actionable 'half' of the modal above the illustrative half?

@mmtr
Copy link
Member Author

mmtr commented Apr 26, 2023

Thanks @BogdanUngureanu, it should be fixed now:

Screenshot 2023-04-26 at 13 35 29 Screenshot 2023-04-26 at 13 35 46

should we display the actionable 'half' of the modal above the illustrative half?

It's intentionally below the image so it looks similar to the Core's welcome modal:

Screenshot 2023-04-26 at 13 31 00

@BogdanUngureanu
Copy link
Contributor

BogdanUngureanu commented Apr 26, 2023

Thanks for the updates, Miguel. I had another look and I've noticed that the scrollbar is still appearing for iPhone 12 pro resolutions.

Maybe for low height resolutions we could:

  • Reduce the padding for wpcom-global-styles-modal__text
  • Remove the margin-top: auto on wpcom-global-styles-modal__actions class, this will move the CTA buttons in the visible screen.

By doing this, we will make sure that the CTA buttons are visible on the screen without scrolling.

@mmtr
Copy link
Member Author

mmtr commented Apr 27, 2023

the scrollbar is still appearing for iPhone 12 pro resolutions.

I cannot reproduce that, maybe you had some caching issues? This is how I see it:

Gutenberg 15.7.0-rc.1 Gutenberg 15.6.2
Screenshot 2023-04-27 at 14 16 37 Screenshot 2023-04-27 at 14 18 16

@BogdanUngureanu
Copy link
Contributor

Oh, sorry about that. The issue is in landscape mode, where the height is small and you have to scroll the modal in order to see the upgrade buttons.

@mmtr
Copy link
Member Author

mmtr commented Apr 27, 2023

Thanks for the extra info. It should be fixed now:

Screenshot 2023-04-27 at 15 34 09

Copy link
Contributor

@BogdanUngureanu BogdanUngureanu left a comment

Choose a reason for hiding this comment

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

Works great on most mobile devices, in landscape mode. I've noticed that for Galaxy Fold, in landscape mode, the buttons are still not visible, but I guess it's an acceptable tradeoff.

@mmtr mmtr merged commit 3f44543 into trunk Apr 28, 2023
@mmtr mmtr deleted the fix/limit-global-styles-modal branch April 28, 2023 08:59
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limited Global Styles: Gating modal has incorrect layout
4 participants