Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anavarre created an issue. See original summary.

anavarre’s picture

Status: Needs review » Needs work

Tests are passing. Putting back to Needs Work per the issue summary.

anavarre’s picture

Issue summary: View changes
anavarre’s picture

Issue summary: View changes
Dom.’s picture

  1. +++ b/config/install/page_load_progress.settings.yml
    @@ -1,2 +1,3 @@
    \ No newline at end of file
    

    newline is also needed on yml files (for instance, see #2521774: Add new line at the EOF in system_test.permissions.yml file).

  2. +++ b/src/Form/SettingsForm.php
    @@ -53,6 +53,13 @@ class SettingsForm extends ConfigFormBase {
    +      '#description' => $this->t('It might happen for the throbber to get stuck in an endless loop. The ESC key acts as a kill switch.'),
    

    Could we change this to something less frightening that "our module may not work" ? Maybe : 'This module locks screen during a form submission process. Under some circunstances, you may want to let your user unlock the screen manually by pressing ESC key.'

Now time to actually use this config ?! ^^

Dom.’s picture

+++ b/js/page_load_progress.js
@@ -14,6 +14,7 @@
+      var esc_key = Boolean(settings.page_load_progress.esc);

+++ b/src/Form/SettingsForm.php
@@ -63,6 +70,7 @@ class SettingsForm extends ConfigFormBase {
+      ->set('page_load_progress_esc_key', $form_state->getValue('page_load_progress_esc_key'))

won't work since hook_page_attachments attachs the config under the name 'esc' while the JS try to get it under name 'esc_key'

Dom.’s picture

anavarre’s picture

Status: Needs review » Needs work
  1. +++ b/js/page_load_progress.js
    @@ -14,15 +14,34 @@
    +      // For each configured locking elements, add the lockscreen behavior.
    

    Not stricly in scope for this issue but what about: "Add the screen lock behavior for each configured element."

  2. +++ b/js/page_load_progress.js
    @@ -14,15 +14,34 @@
    +      // If screen lock should be killable via ESC key press, add a keypress
    

    Can we simplify this to: "Allows ESC key to kill the throbber"?

  3. +++ b/js/page_load_progress.js
    @@ -14,15 +14,34 @@
    +          if (e.keyCode == 27) {
    

    Looks like doing this might be outdated. See http://stackoverflow.com/questions/3369593/how-to-detect-escape-key-pres...

  4. +++ b/js/page_load_progress.js
    @@ -14,15 +14,34 @@
    +       * Lock screen method.
    

    Not sure about the coding standards for this JS dockblock. Any idea if we're missing a new line or anything?

  5. +++ b/src/Form/SettingsForm.php
    @@ -53,6 +53,13 @@ class SettingsForm extends ConfigFormBase {
    +      '#description' => $this->t('This module locks screen during a form submission process. Under some circunstances, you may want to let your user unlock the screen manually by pressing ESC key.'),
    

    I agree the initial verbiage might have been too strong. I like your wording better, but I'd drop "This module locks screen during a form submission process." completely because it's essentially what the whole module is doing.

anavarre’s picture

FileSize
2.5 KB

Also, re-uploading the interdiff for better readability.

anavarre’s picture

FYI I've just tested the patch against D8, to reproduce #1818872: Cant edit views with .form-submit element enabled. and it works great. I can now easily kill the throbber when editing a view makes the throbber spins forever.

  • anavarre committed 274ef67 on 8.x-1.x authored by Dom.
    Issue #2779741 by Dom., anavarre: There should be a way to kill the...
anavarre’s picture

Status: Needs review » Fixed

Tested the patch and it works well. Backwards compatibility is also good to have. This is what we need for now. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.