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

Standardize on WordPress dismiss button #193

Closed
raamdev opened this Issue Dec 26, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@raamdev
Contributor

raamdev commented Dec 26, 2015

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>

@raamdev raamdev added the enhancement label Dec 26, 2015

@raamdev raamdev added this to the Next Release milestone Dec 26, 2015

@raamdev raamdev changed the title from Standardize on WordPress dismissable notice format to Standardize on WordPress dismissable notice button Dec 26, 2015

@raamdev raamdev changed the title from Standardize on WordPress dismissable notice button to Standardize on WordPress dismiss button Dec 26, 2015

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 29, 2015

👍

@kristineds

This comment has been minimized.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Jan 2, 2016

@kristineds Yes, I'd love to see a PR for this. 😄 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.

@raamdev raamdev modified the milestones: Future Release, Next Release Jan 21, 2016

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 25, 2016

@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 kristineds self-assigned this Jan 31, 2016

@kristineds

This comment has been minimized.

Contributor

kristineds commented Feb 6, 2016

@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

This comment has been minimized.

Member

jaswrks commented Feb 6, 2016

@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

This comment has been minimized.

Contributor

kristineds commented Feb 8, 2016

@jaswsinc I added the classes button.notice-dismiss and span.screen-reader-text but the formatting needs to be fixed for the dashicon . #225 (comment)

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

This comment has been minimized.

Member

jaswrks commented Feb 8, 2016

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. #193 (comment)

<div class="comment-mail-menu-page-area updated notice is-dismissible"
@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 19, 2016

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

@raamdev

This comment has been minimized.

Contributor

raamdev commented Mar 18, 2016

Next Release Changelog:

  • UI Enhancement: Dashboard notices generated by Comment Mail now use the WordPress-style dismiss button to keep things consistent. Props @kristineds. See Issue #193.

@raamdev raamdev closed this Mar 18, 2016

raamdev added a commit that referenced this issue Jun 18, 2016

Phing release of v160618 with the following changes:
- **Restructured Codebase**: The codebase has been completely restructured to improve performance, enhance flexibility, and make it easier to build in new features! Props @jaswsinc. See [Issue #150](#150).
- **Comment Mail Pro Upgrade Notice: Incompatible Advanced Templates.** This version of Comment Mail includes a rewritten and improved codebase. This rewrite, however, came with the unfortunate side effect of breaking backwards compatibility with Advanced Templates that were customized in a previous version of Comment Mail Pro.

     If you are currently using Comment Mail Pro and you've customized your Advanced Templates, all of your customized Advanced Templates will be backed up and the templates will then be reset to their new defaults. You will find the backup of your old customized template appended to the bottom of the new template, separated with a  <code>Legacy Template Backup</code> PHP comment. See [example screenshots](#238 (comment)).

     Note: This change has no effect on Simple templates—only Advanced Templates are affected. Advanced Templates are a Pro-only feature, so this notice only applies to Comment Mail Pro. See [Issue #238](#238).
- **Bug Fix**: Fixed a bug where `esc_html()` was being used where `esc_sql()` should've been used. Props @jaswsinc @kristineds. See [Issue #268](#268).
- **Bug Fix**: Fixed a bug that in some scenarios resulted in a "DB table creation failure" error when activating the plugin. Props @thienhaxanh2405, @PanNovak, @kristineds, and @jaswsinc. See [Issue #260](#260).
- **Bug Fix**: Fixed a bug where "New reply" notification emails were not being parsed properly by some Hotmail accounts and were showing up as blank. Props @kristineds. See [Issue #259](#259).
- **Bug Fix**: Fixed a bug that allowed spam comments to create subscriptions in Comment Mail when using Akismet. Props @IvanRF. See [Issue #250](#250).
- **Bug Fix** (Pro): When Chrome or Firefox Autofill Username/Password was enabled, the Comment Mail Pro Updater fields would incorrectly be autofilled by the browser with invalid credentials. This has been fixed. Props @renzms. [Issue #274](#274).
- **Bug Fix**: Fixed a bug where the cron job for the Queue Processor could get deleted and never recreated, which would result in notifications getting stuck in the Mail Queue and not being sent out. If you ever installed Comment Mail and then deleted it (without first disabling Data Safeguards), and then installed Comment Mail again, you were probably affected by this issue. This release fixes the issue and makes the cron setup more robust. Props @kristineds, @renzms, @jaswsinc, and @IvanRF for help testing. See [Issue #194](#194) and [Issue #173](#173).
- **Bug Fix:** Fixed a bug where a subscriber who selected Hourly Digest and who had never been notified before could, in some scenarios, have their subscription treated instead as a Weekly Digest. This bug was found and fixed during the codebase restructuring. Props @jaswsinc. See [Issue #150](#150) and additional discussion in [Issue #173](#173 (comment)).
- **Bug Fix:** Fixed a bug where in some scenarios Mail Queue entries for Digest Notifications that should have been held for sending later were not being held and were also not being sent. They also would not have shown up in the Mail Queue Event Log. This bug was found and fixed during the codebase restructuring. Props @jaswsinc. See [Issue #150](#150) and additional discussion in [Issue #173](#173 (comment)).
- **Enhancement**: Minor improvements to the Options Page menu links and positioning of the Pro Preview link. Props @renzms. See [Issue #227](#227).
- **Enhancement**: It's now possible to use the following shortcodes in the Email Footer Tag for Email Footer Templates: `[home_url]`, `[blog_name_clip]`, and `[current_host_path]`. Props @kristineds and @IvanRF. See [Issue #246](#246).
- **Enhancement**: Improved the Subscriptions meta box that appears on the Post Edit screen. For each subscription, the meta box now lists the full name and email address, the date the subscription was created, and a view link that allows you to view/edit the subscription. Props @kristineds. See [Issue #231](#231).
- **UX Enhancement (Pro)**: Improved the Dashboard notice that appears when you try to enable the Pro version of Comment Mail when the Lite version is currently enabled. Props @kristineds @jaswsinc. See [Issue #230](#230).
- **UX Enhancement**: When Subscribing Without Commenting, the Add New Subscription form now pre-populates the Name and Email address fields whenever possible. Props @kristineds. See [Issue #204](#204).
- **UI Enhancement**: Dashboard notices generated by Comment Mail now use the WordPress-style dismiss button to keep things consistent. Props @kristineds. See [Issue #193](#193).
@raamdev

This comment has been minimized.

Contributor

raamdev commented Jun 19, 2016

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).

@websharks websharks locked and limited conversation to collaborators Jun 19, 2016

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