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

Classic editor and Cloudflare notification modals improvements #9430

Merged
merged 2 commits into from Aug 31, 2018

Conversation

Projects
None yet
3 participants
@afercia
Contributor

afercia commented Aug 29, 2018

Description

This PR:

  • sets initial focus within the Cloudflare modal (the selector used to target the button was wrong)
  • adjust the CSS to make the modals vertically and horizontally centered

Note:
the CSS is partially inherited from WP, please do let me know if it's better to replicate all the styles used and add them to compat.php

To test, you need to make the modals appear:

  • for the Cloudflare one, maybe the quickest way is to alter the code in here and change the check to !==

    const cloudflaredMessage = error && 'cloudflare_error' === error.code ?

  • create a post, enter some content

  • then, set Chrome dev tools > Network to "offline"

  • see a notice appears with text related to Cloudflare and a link:

screen shot 2018-08-29 at 14 46 17

  • click the "Learn more" link
  • a new edit post page opens in a new browser tab, with the Cloudflare modal displayed
  • for the Classic editor modal just edit in the Classic editor a post created in Gutenberg
  • verify the Cloudflare modal button gets initial focus
  • verify both modals are vertically and horizontally centered
  • verify content is always visible at various viewport sizes

Cloudflare modal before:

screen shot 2018-08-29 at 13 47 24

Cloudflare modal after:

screen shot 2018-08-29 at 14 47 49

Fixes #9428

@afercia afercia requested a review from pento Aug 29, 2018

@tofumatt

Looks good to me. This would be good to land in 3.7 as I'd guess plenty of users see this.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 30, 2018

Contributor

@tofumatt thanks. My only doubt is about the CSS inherited by the core edit.css (they might change at any time). Do you think it would be safer to copy those rules into compat.php?

Contributor

afercia commented Aug 30, 2018

@tofumatt thanks. My only doubt is about the CSS inherited by the core edit.css (they might change at any time). Do you think it would be safer to copy those rules into compat.php?

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 30, 2018

Member

Oh, right, sorry!

Probably best to copy over rules, if there aren’t that many, just to be safe. Feel free to ping me for a re-review after that if you need.

Member

tofumatt commented Aug 30, 2018

Oh, right, sorry!

Probably best to copy over rules, if there aren’t that many, just to be safe. Feel free to ping me for a re-review after that if you need.

@gziolo gziolo added this to the 3.8 milestone Aug 30, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 30, 2018

Contributor

Copied the styles. Ping @tofumatt

Contributor

afercia commented Aug 30, 2018

Copied the styles. Ping @tofumatt

@tofumatt

Seems good to me! Some of these core styles are a bit intense (z-index), but seems good.

background: #fff;
box-shadow: 0 3px 6px rgba(0, 0, 0, 0.3);
line-height: 1.5;
z-index: 1000005;

This comment has been minimized.

@tofumatt

tofumatt Aug 30, 2018

Member

Ouch! 😓

@tofumatt

tofumatt Aug 30, 2018

Member

Ouch! 😓

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 30, 2018

Contributor

Don't ask 😬z-index in core is fun 🎉

Contributor

afercia commented Aug 30, 2018

Don't ask 😬z-index in core is fun 🎉

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 30, 2018

Contributor

Unrelated failing e2e tests are a bit annoying... 😬

Contributor

afercia commented Aug 30, 2018

Unrelated failing e2e tests are a bit annoying... 😬

@afercia afercia merged commit 9f2b3e3 into master Aug 31, 2018

2 checks passed

codecov/project 50.23% remains the same compared to c5573b7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@afercia afercia deleted the update/classic-editor-cloudflare-notification-modals-improvements branch Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment