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

Add message if JavaScript is disabled #11642

Merged
merged 9 commits into from Nov 13, 2018

Conversation

@mkaz
Member

mkaz commented Nov 8, 2018

Description

Adds a noscript and message if Javascript is disabled.

This also required adding a :not(.no-js) to the fullscreen body class
which hides/shows the rest of the wp-admin chrome.

How has this been tested?

  • Disable Javascript using console settings
  1. Prior to change confirm the page is blank.
  2. After change you should see the message

screen shot 2018-11-08 at 11 23 34 am

Types of changes

See #2719

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

mkaz added some commits Nov 8, 2018

Add message if Javascript is disabled
Adds a noscript and message if Javascript is disabled.

This also required adding a :not(.no-js) to the fullscreen body class
which hides/shows the rest of the wp-admin chrome.
<noscript>
<div class="error" style='margin-top:32px'><p>
<?php
echo __( 'The block editor requires Javascript to be enabled.', 'gutenberg' );

This comment has been minimized.

@ocean90

ocean90 Nov 8, 2018

Member

echo __() => _e()

This comment has been minimized.

@mkaz

mkaz Nov 8, 2018

Member

The echo __() function is used in numerous spots in gutenberg.php, I just mirrored it to stay consistent within the same file.

This comment has been minimized.

@mkaz

mkaz Nov 9, 2018

Member

Thumbs down is not really a productive response.
I've switched each of the occurrences to the shorter function.

@@ -29,6 +29,13 @@ function the_gutenberg_project() {
global $post_type_object;
?>
<div class="block-editor gutenberg">
<noscript>
<div class="error" style='margin-top:32px'><p>

This comment has been minimized.

@ocean90

ocean90 Nov 8, 2018

Member

The admin bar has different heights depending on the viewport.

This comment has been minimized.

@mkaz

mkaz Nov 8, 2018

Member

The margin was simply to give some spacing from the bottom of the admin bar, so the error message isn't squished up against the top.

@@ -1,4 +1,4 @@
body.is-fullscreen-mode {

This comment has been minimized.

@swissspidy
@youknowriad

LGTM 👍

@michelleweber

This comment has been minimized.

michelleweber commented Nov 12, 2018

I always like to give a specific next step in an error message when possible. In this case, that would be something like "The Block Editor requires Javascript. Go to your browser's settings and enable Javascript before using it. Learn more."

Ideally the "learn more" is a link to instructions re: how to enable JS, but if we don't have that, just delete it.

@chrisvanpatten

Small typo!

Show resolved Hide resolved gutenberg.php Outdated
@mkaz

This comment has been minimized.

Member

mkaz commented Nov 12, 2018

@michelleweber I've updated with your change, but after revisiting the bug, I think the suggestion here by @afercia might be better, adding a to link to the Classic Editor plugin

The user probably either has JavaScript disabled on purpose, or it not supported by their device, so the plugin instructions might be more helpful.

@michelleweber

This comment has been minimized.

michelleweber commented Nov 12, 2018

Works for me. As long as we give the user some kind of next step, we're good -- I leave it to you to decide what the actual best next step is!

@swissspidy

This comment has been minimized.

Member

swissspidy commented Nov 12, 2018

I know this is now getting a bit more complex, but can we add some more conditions whether the Classic Editor plugin is already installed and whether it's active? Perhaps also whether the user has the capability to install it.

At least a direct link to the classic editor if the plugin is already active would be nice.

Fix link not be clickable
Add position absolute and higher z-index to make link clickable.
The block-editor pane was positioned above the inserted link.

h/t @georgehotelling
@mkaz

This comment has been minimized.

Member

mkaz commented Nov 12, 2018

@swissspidy I know, so much effort on a rare edge case, but might as well do it properly.

I did consider that about if the plugin is installed, however the user should not get to this page since it is a Gutenberg page and if the plugin is installed they will go straight to the Classic Editor.

I also just had to push up some CSS z-index positioning stuff to make it clickable, no such thing as a quick fix.

@afercia

This comment has been minimized.

Contributor

afercia commented Nov 12, 2018

@mkaz thanks for working on this!

Seems it's not possible to actually click on the link because the #editor container overlays the notice 🙂

@jasmussen is printing out by default is-fullscreen-mode via PHP really necessary? I guess there are JS ways to set it before everything is rendered.

@mkaz

This comment has been minimized.

Member

mkaz commented Nov 12, 2018

@afercia can you make sure you are updated to the latest PR, I just pushed up a fix to address the issue of it being clickable. Also, just updated with your CSS suggestion.

@swissspidy

This comment has been minimized.

Member

swissspidy commented Nov 12, 2018

What if the plugin is installed but not active?

@afercia

This comment has been minimized.

Contributor

afercia commented Nov 12, 2018

The link is clickable now, thanks!

@karmatosed karmatosed self-requested a review Nov 12, 2018

@@ -28,6 +28,16 @@
function the_gutenberg_project() {
global $post_type_object;
?>
<noscript>
<div class="error" style='position:absolute;top:32px;z-index:40'><p>

This comment has been minimized.

@GaryJones

GaryJones Nov 13, 2018

Contributor

Why use single quotes for the style attribute?

<div class="error" style='position:absolute;top:32px;z-index:40'><p>
<?php
printf(
__( 'The Block Editor requires JavaScript, please try the <a href="%s">Classic Editor plugin</a>.', 'gutenberg' ),

This comment has been minimized.

@GaryJones

GaryJones Nov 13, 2018

Contributor

This will read better as two sentences.
"The Block Editor requires JavaScript. Please try the Classic Editor plugin."

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 13, 2018

The Block Editor requires JavaScript. Please try the Classic Editor plugin.

I think that this message is valid in the context of WordPress core. I'm not convinced that it is the way to go when Gutenberg plugin is activated. @mcsf @youknowriad @mtias do you agree?

@mcsf

This comment has been minimized.

Contributor

mcsf commented Nov 13, 2018

The Block Editor requires JavaScript. Please try the Classic Editor plugin.

I think that this message is valid in the context of WordPress core. I'm not convinced that it is the way to go when Gutenberg plugin is activated.

Yeah. Might be behind a conditional:

if ( is_plugin_active( 'gutenberg/gutenberg.php' ) ) {
  // The Block Editor requires JavaScript. Please enable JavaScript or use the Classic Editor.
} else {
  // The Block Editor requires JavaScript. Please try the Classic Editor plugin.
}

In the first version, "Classic Editor" links to the current URL with an extra classic-editor query argument. In the second version, "Classic Editor plugin" links to the actual plugin.

I don't feel strongly about keeping the "enable JavaScript" fragment, but it seems fair to ask for this if a user went to the trouble of installing the Gutenberg plugin.

@swissspidy swissspidy changed the title from Add message if Javascript is disabled to Add message if JavaScript is disabled Nov 13, 2018

Add conditional for plugin vs core
Two different messages if trying to access block editor via the
plugin or via core.
@mkaz

This comment has been minimized.

Member

mkaz commented Nov 13, 2018

@mcsf @gziolo - I've updated based on your suggestions.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 13, 2018

Thanks @mkaz for updating 👍

Show resolved Hide resolved gutenberg.php Outdated
@mkaz

This comment has been minimized.

Member

mkaz commented Nov 13, 2018

@chrisvanpatten can I get your on the resolved typo? Thanks!

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Nov 13, 2018

@mkaz Oh sorry, I thought my review would have been auto-dismissed after you incorporated my suggestion. Get that fixed up, GitHub!

Approving in a sec :)

@mkaz mkaz merged commit ee51293 into master Nov 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mkaz

This comment has been minimized.

Member

mkaz commented Nov 13, 2018

Thanks @chrisvanpatten I marked as resolved too, I do think it's a Github bug.

@mkaz mkaz deleted the fix/2719/no-javascript branch Nov 13, 2018

@mkaz mkaz referenced this pull request Nov 13, 2018

Merged

Fix code style issues introduced in #11642 #11829

4 of 4 tasks complete

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Add message if JavaScript is disabled (WordPress#11642)
* Add message if Javascript is disabled

Adds a noscript and message if Javascript is disabled.

This also required adding a :not(.no-js) to the fullscreen body class
which hides/shows the rest of the wp-admin chrome.

* PHP linting

* 12 bytes saved

* Fix camel-case, update with Copy Review

* Switch message to include link to Classic Editor plugin

* Fix link not be clickable

Add position absolute and higher z-index to make link clickable.
The block-editor pane was positioned above the inserted link.

h/t @georgehotelling

* Use existence of .js class instead of :not(.no-js)

h/t to @afercia

* Add conditional for plugin vs core

Two different messages if trying to access block editor via the
plugin or via core.

* Use add_query_arg()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment