wpsharks / comment-mail

A WordPress plugin enabling email subscriptions for comments.
http://comment-mail.com
GNU General Public License v3.0
8 stars 3 forks source link

Standardize on WordPress dismiss button #193

Closed raamdev closed 8 years ago

raamdev commented 8 years ago

WordPress Core uses dismissable Dashboard notices and already has a specific style for them. Comment Mail (and other WebSharks plugins) diverge from this style and instead use a "dismiss x" style.

I feel this divergence from the norm is unnecessary and can lead to confusion: WordPress users are used to looking for a specific thing when trying to dismiss a notice; why introduce a new way of doing something that is already well-understood?

Also, our dismiss button is not compatible with screen readers (for those with disabilities).

2015-12-26_10-11-14

Here's the HTML Source for the "Plugin activated" notice shown above:

<div id="message" class="updated notice is-dismissible">
     <p>Plugin <strong>activated</strong>.</p>
     <button type="button" class="notice-dismiss">
          <span class="screen-reader-text">Dismiss this notice.</span>
     </button>
</div>
jaswrks commented 8 years ago

:+1:

kristineds commented 8 years ago

@raamdev @jaswsinc Can I work on this? Should I tweak it here? https://github.com/websharks/comment-mail-pro/blob/28bbe8b88b59a77e3632938d5c72314ff86488a2/comment-mail-pro/includes/stcr.php#L96

raamdev commented 8 years ago

@kristineds Yes, I'd love to see a PR for this. :smile: This tweak will require understanding how the Comment Mail notice utilities work and how they produce the dismiss button, etc. See this section.

Then, if you get a WordPress Core-generated dismissable notice to appear (such as the Plugin activated notice), you can reverse engineer that to see how it works (using the Chrome inspector) and then adjust Comment Mail's notice utilities accordingly.

jaswrks commented 8 years ago

@kristineds Here is the specific markup that needs to get changed to what Raam noted above. See: https://github.com/websharks/comment-mail/blob/151224/comment-mail/plugin.inc.php#L1998-L2001

That needs to get updated to markup that takes advantage of built-in WordPress styles.

<button type="button" class="notice-dismiss">
    <span class="screen-reader-text">Dismiss this notice.</span>
</button>

This might require further tweaking, but if you can work on this particular markup and run a test or two, open a PR, post your test results; and then we can work toward achieving this a little bit at a time through additional collaboration or tweaks.

kristineds commented 8 years ago

@jaswsinc I was trying to do this but I'm having a hard time. How do I pull the icons for WordPress? I read up and it says it uses dashicons: https://developer.wordpress.org/resource/dashicons/#dismiss. Is that already part of the Comment Mail codebase? Do I need to do anything special on the CSS file to call it?

jaswrks commented 8 years ago

@kristineds As I understand it, the HTML markup alone should be enough. The WordPress core should already have the styles set for the classes button.notice-dismiss and span.screen-reader-text. When you added that markup to a notice, did those styles not take effect properly, or at all?

Sorry, I haven't worked with those specific classes myself yet. You shouldn't need to pull Dashicons in to accomplish this though. The point of using those classes with that specific markup would be for us to take advantage of and match the existing UI styles in WordPress core.

kristineds commented 8 years ago

@jaswsinc I added the classes button.notice-dismiss and span.screen-reader-text but the formatting needs to be fixed for the dashicon . https://github.com/websharks/comment-mail/pull/225#issue-132172128

That whole div for the notice is using the class comment-mail-menu-page-area updated. Should I try overriding that? Or try to create my own class?

screen shot 2016-02-08 at 11 16 20 pm

jaswrks commented 8 years ago

That whole div for the notice is using the class comment-mail-menu-page-area updated. Should I try overriding that? Or try to create my own class?

I think it should have the additional parent classes Raam showed here. https://github.com/websharks/comment-mail/issues/193#issue-123934285

<div class="comment-mail-menu-page-area updated notice is-dismissible"
jaswrks commented 8 years ago

@kristineds There are specific details here about how this works. https://codex.wordpress.org/Plugin_API/Action_Reference/admin_notices

raamdev commented 8 years ago

Next Release Changelog:

raamdev commented 8 years ago

Comment Mail v160618 has been released and includes changes from this GitHub Issue. See the v160618 announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#193).