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

"Dismiss" button not persistently dismissing notices #14

Closed
pjohanneson opened this issue Feb 17, 2021 · 4 comments · Fixed by #15
Closed

"Dismiss" button not persistently dismissing notices #14

pjohanneson opened this issue Feb 17, 2021 · 4 comments · Fixed by #15

Comments

@pjohanneson
Copy link

I found I would dismiss notices, but on reloading the page they'd be back. The option wasn't being set in the site's options table.

Further investigation showed that the javascript console was seeing an error: dismissBtn was coming back null.

I changed the file src/Dismiss.php from this:

var dismissBtn  = document.querySelector( '#wptrt-notice-<?php echo esc_attr( $this->id ); ?> .notice-dismiss' );

to this:

var dismissBtn  = document.querySelector( '#wptrt-notice-<?php echo esc_attr( $this->id ); ?>' );

...and it worked. It appears that the class .notice-dismiss isn't being added.

I'm running WordPress 5.6.1.

@pjohanneson
Copy link
Author

pjohanneson commented Feb 17, 2021

Hmm, on further investigation this might be a browser bug. It works fine in Chrome (88.0.4324.150), but not in Firefox (85.0.1).

@BrianHenryIE
Copy link

It's not a browser bug, it's a race condition.

The HTML generated is just a DIV with the relevant classes:

'<div id="%1$s" class="%2$s">%3$s</div>',

WordPress's common.js then adds the dismiss button to the div.

WPTRT admin-notices's JavaScript is printed on admin_footer priority 10. It needs to somehow be configured to run after the common.js code, which itself runs inside $( function( $ ) { ... });

I tried to use wp_register_script( $handle, '', array( 'common' ), $version, true ); but it still didn't work as hoped.

What does work is to wrap the existing JavaScript in a timer:

setTimeout(function(){ 
  // Existing code.
}, 500);

Or to use a MutationObserver to watch until the .notice-dismiss has been added, and run the code as normal. I have this in a fork but I don't think it's the best way to do it: BrianHenryIE/admin-notices

Looking again at common.js, it's using .on( ) to initially add the buttons.

I toyed around a little with that too – trying to use similar JavaScript but loaded after common.js – but didn't get it working. Maybe someone handy with JavaScript could suggest how to achieve it.

@BrianHenryIE
Copy link

I worked on this again yesterday. I'll be testing this change: master...BrianHenryIE:admin-notices:fix-race-condition-#14

The changes are mostly:

  • Use jQuery( function() { rather than window.addEventListener( 'load', function() {
  • Use wp_add_inline_script hooked on admin_enqueue_scripts after common JS rather than just echo on admin_footer

@BrianHenryIE
Copy link

Because of #18 I'm now using an unpatched version of the library and can confirm the issue is still present with Firefox.

@kafleg kafleg closed this as completed in #15 Oct 6, 2023
kafleg added a commit that referenced this issue Oct 6, 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 a pull request may close this issue.

2 participants