Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#31233 closed task (blessed) (fixed)

Dismissable admin notices

Reported by: ryan's profile ryan Owned by: helen's profile helen
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.1
Component: Administration Keywords: has-patch
Focuses: ui, accessibility Cc:

Description (last modified by iseulde)

Let's make admin notices dismissable.

https://make.wordpress.org/flow/2015/01/29/focus-and-admin-notices-macnchrome/

Admin notices result in empty space near the top of the screen when Focus is activated. Some notices, such as the upgrade network notice, do not fade away when Focused. Focus must reserve space for the notices so that they can be winked back in when unfocused. I’ve long wanted dismissable admin notices. Accommodating Focus seems like a good motivation to finally make notices dismissable. After saving a draft, being stuck with a padded header while Focused is annoying. Being able to click to dismiss notices would workaround this (rewriting the url to get rid of the message query arg would also be nice) .

Attachments (42)

31233.diff (1.6 KB) - added by helen 9 years ago.
31233.right.diff (2.2 KB) - added by helen 9 years ago.
31233.2.diff (3.5 KB) - added by valendesigns 9 years ago.
31233.3.diff (3.0 KB) - added by valendesigns 9 years ago.
Screen Shot 2015-03-16 at 10.25.02 AM.png (464.4 KB) - added by ryan 9 years ago.
31233.3.diff, Macnchrome
IMG_3311.PNG (78.0 KB) - added by ryan 9 years ago.
31233.3.diff, iPhone 5
IMG_0789.PNG (163.9 KB) - added by ryan 9 years ago.
31233.3.diff, iPhone 6+
Screenshot_2015-03-16-10-37-34.png (176.9 KB) - added by ryan 9 years ago.
31233.3.diff, Nexus 5
31233.4.diff (3.3 KB) - added by valendesigns 9 years ago.
IMG_0790.PNG (163.8 KB) - added by ryan 9 years ago.
31233.4.diff, iPhone 6+
31233.5.diff (3.3 KB) - added by valendesigns 9 years ago.
iPad-4.PNG (155.5 KB) - added by valendesigns 9 years ago.
iPhone-5.PNG (82.5 KB) - added by valendesigns 9 years ago.
31233.6.diff (3.4 KB) - added by valendesigns 9 years ago.
Fixes FireFox
31233.7.diff (3.4 KB) - added by adamsilverstein 9 years ago.
31233.8.diff (3.4 KB) - added by valendesigns 9 years ago.
31233.9.diff (3.7 KB) - added by valendesigns 9 years ago.
31233.10.diff (3.8 KB) - added by valendesigns 9 years ago.
31233.11.diff (3.8 KB) - added by valendesigns 9 years ago.
31233.12.diff (4.2 KB) - added by valendesigns 9 years ago.
31233.13.diff (4.1 KB) - added by adamsilverstein 9 years ago.
add missing comma
31233.14.diff (4.4 KB) - added by afercia 9 years ago.
31233.15.diff (4.3 KB) - added by paulwilde 9 years ago.
31233.16.diff (5.1 KB) - added by valendesigns 9 years ago.
Chrome-41.png (157.5 KB) - added by valendesigns 9 years ago.
Chrome-41-focus.png (158.7 KB) - added by valendesigns 9 years ago.
Firefox-36.png (156.7 KB) - added by valendesigns 9 years ago.
Opera-12.png (149.7 KB) - added by valendesigns 9 years ago.
IE-10.png (143.2 KB) - added by valendesigns 9 years ago.
iPad-4.png (156.6 KB) - added by valendesigns 9 years ago.
iPhone-5.png (82.1 KB) - added by valendesigns 9 years ago.
31233.17.diff (5.1 KB) - added by valendesigns 9 years ago.
fix whitespace
31233.18.diff (5.2 KB) - added by valendesigns 9 years ago.
Forgot type="button"
31233.19.diff (5.1 KB) - added by valendesigns 9 years ago.
31233.20.diff (4.5 KB) - added by valendesigns 9 years ago.
31233.21.diff (4.5 KB) - added by valendesigns 9 years ago.
fix typo
31233.21.2.diff (1.1 KB) - added by valendesigns 9 years ago.
31233.22.diff (1.1 KB) - added by valendesigns 9 years ago.
wp-screen.png (19.5 KB) - added by miqrogroove 9 years ago.
Feature Bugged in Beta 4 :(
31233.23.diff (1.2 KB) - added by valendesigns 9 years ago.
refresh
31233.24.diff (1.1 KB) - added by valendesigns 9 years ago.
removed whitespace added during copy & paste
31233.25.diff (1.2 KB) - added by valendesigns 9 years ago.

Change History (111)

#1 @ryan
9 years ago

Related: #23367

This ticket was mentioned in Slack in #design by boren. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-flow by boren. View the logs.


9 years ago

#5 @iseulde
9 years ago

  • Description modified (diff)

I agree. I even think that messages such as "post saved" should disappear after a certain time, but that may be be a bigger step. Would it be okay if the whole screen moves up when dismissing a message? We talked about this earlier in #29386, though that's about automatic removal. Still, for the same reason it's really difficult to add any additional messages with JS at a later point. The whole screen would move down while you may be typing. It might be better to have have floating messages in a corner. Going off topic now, should probably be discussed in a new ticket. :)

#6 follow-up: @helen
9 years ago

I think it's okay if the screen moves up, notices are at the top and you'd be focused on looking at them. A first run where you can just dismiss for that particular page load seems like it'd be fine.

#7 in reply to: ↑ 6 @ryan
9 years ago

Replying to helen:

I think it's okay if the screen moves up, notices are at the top and you'd be focused on looking at them. A first run where you can just dismiss for that particular page load seems like it'd be fine.


Feeling the same. My focus would be on the notice as I click/tap to dismiss it. The screen moving up feels like an expected reaction that doesn't violate the sanctity of scroll position. It could be welcoming to have the screen move up to meet you when the annoying notice is sent away. We'll see. I'm interested in seeing what ends up under the mouse pointer when the notice is dismissed. Does it put you right on top of Preview? Is that good? Would accidental double clicks on the dismiss button result in unexpected new tab generation as preview is unintentionally triggered? I'm just casting about for possible problems. Let's run with that first run.

Last edited 9 years ago by ryan (previous) (diff)

@helen
9 years ago

#8 @helen
9 years ago

31233.diff is just a starting point to see how we feel about it - doesn't style the link or anything, and I guess it should probably be a button element per a11y folks. In any case, I agree with ryan's concern about where it leaves your pointer if you're using a mouse or tapping - could be interesting to try putting it on the left side instead of the right, since that's where you're reading the notice in the first place.

This ticket was mentioned in Slack in #core by morganestes. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-flow by helen. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by helen. View the logs.


9 years ago

@helen
9 years ago

#12 @helen
9 years ago

  • Milestone changed from Awaiting Review to 4.2
  • Type changed from enhancement to task (blessed)

#23367 went in, so let's get this done in 4.2, at least for core notices on a per page load basis.

@valendesigns
9 years ago

#13 follow-up: @valendesigns
9 years ago

  • Keywords has-patch dev-feedback added

The last patch had a minor UI issue where it would cover a bit of text in long messages. I fixed that by prepending the link and floating it right instead of appending and making it absolute positioned.

Once I fixed that, I saw a potential for making the whole thing much better. My implementation in patch 31233.2.diff includes hide, but also adds dismiss with cookie support. The dismiss functionality is only activated when the notice has data-dismiss="true" set. This way we could go beyond per page load and actually have legitimate dismissible notices, not just hide them.

I also made the cookie expiration extensible with a data attribute. Instead of the cookie expiring after the session you can set that number using data-expires="60", for example. Then the notice will only be dismissed for a minute.

Something I think is important is the ability to turn the default hide behavior off on a per notice basis. So if you add data-hide="false" that notice will always display. I still have some backwards compatibility concerns with adding this to every single notice. What about all the plugins and themes that already have this type of functionality? Well, those notices would be really ugly and confusing. So at the very least, we need to be able to turn this off. I suggest we also post on the Make blog about any change to notices, and give plugin & theme developers time to update their code.

Here are some notice examples to further demonstrate my changes.

function ticket_31233_admin_notices() {
	?>
	<div class="updated">
		<p><?php _e( 'This notice can be hidden for a single page load, and is the default behavior.' ); ?></p>
	</div>
	<div class="updated" data-hide="false">
		<p><?php _e( 'This notice cannot be hidden.' ); ?></p>
	</div>
	<div class="updated" data-dismiss="true">
		<p><?php _e( 'This notice can be dismissed for the entire session.' ); ?></p>
	</div>
	<div class="updated" data-dismiss="true" data-expires="60">
		<p><?php _e( 'This notice can be dismissed for 60 seconds.' ); ?></p>
	</div>
	<?php
}
add_action( 'admin_notices', 'ticket_31233_admin_notices' );

#14 in reply to: ↑ 13 @helen
9 years ago

Replying to valendesigns:

The last patch had a minor UI issue where it would cover a bit of text in long messages.

Wasn't meant to be a tested patch, I just dump work as patches onto tickets sometimes because I have a bash script that uploads them directly :) If it's a viable patch, I'll explain it in a comment. I decided the method wasn't viable for a couple of reasons, including UI, but wanted the record of "this was tried".

Once I fixed that, I saw a potential for making the whole thing much better. My implementation in patch 31233.2.diff includes hide, but also adds dismiss with cookie support.

It is super cool that you've done this, and being able to actually dismiss them would definitely be a longer term goal. For 4.2, that cannot happen now that we're in beta as that falls squarely into enhancement territory. Let's hold on to your patch, though, for sure.

The reason why I want to be able to "hide" notices for this release is because #23367 went in, so I think it's a reasonable feature extension to be able to hide notices that are not going to show up again when you reload the page or navigate elsewhere, given the bad UI it causes on post edit. That's another big reason the earlier approach of JS insertion isn't a good one right now - we should only do these for core notices/messages in 4.2, and so we can do it in PHP. Maybe we only do it for post edit messages this release, even. To that end, I think we can go with an X icon (with screen reader text), at least.

And finally, let's not forget that notices are moved with JS which is terrible (see this comment on #30885. If we're prepared to go down this road of persistently dismissable notices for all, we should probably think about making adding notices a little less of a manual process - see #12738 (wontfixed right now, but we can always change our minds).

@valendesigns
9 years ago

#15 @valendesigns
9 years ago

@helen I didn't expect my last patch to make it into 4.2, but the idea of using a cookie was something I felt like pursuing for fun anyhow. So it wasn't a complete waste of time. Glad to hear you liked the concept though.

Patch 31233.3.diff adds hide functionality to the post edit messages and uses a dashicon X with screen reader text. I think this should solve the immediate issue for the post edit screen, although it still doesn't support any of the global nags that would cause empty space in DFW mode. From what I gather you're not concerned with those right now in 4.2, just the ones that appear for a single page load. Let me know what you think. Cheers!

#16 @afercia
9 years ago

From an accessibility point of view I'd recommend:

  • please don't introduce new non-link links (href="#"), we're going to remove them through all the admin screens, see #26504. This should really be a button, see for example the "Insert/edit link" modal
  • ideally, in the markup the button should be after the message, users need to know "what" they're going to dismiss before landing on a control that just says "hide", absolute positioning would be OK if done properly
  • the screen-reader-text "Hide" doesn't make much sens for blind people :) maybe something like "Dismiss this notice" would be more appropriate
  • minor: consider to make the clickable area bigger

Will ask the accessibility team if an "alertdialog" ARIA role would be appropriate for these dismissable notices, will let you know. Thanks very much (really like the idea here!).

#17 @afercia
9 years ago

  • Focuses accessibility added

@ryan
9 years ago

31233.3.diff, Macnchrome

@ryan
9 years ago

31233.3.diff, iPhone 5

@ryan
9 years ago

31233.3.diff, iPhone 6+

@ryan
9 years ago

31233.3.diff, Nexus 5

#18 @valendesigns
9 years ago

Hey Guys, I'll work up a new patch now to address the previous concerns. I've got some time.

@valendesigns
9 years ago

#19 @valendesigns
9 years ago

Patch 31233.4.diff should address the accessibility issues from @afercia and mobile UI issue from @ryan. However, I don't know how to test the mobile UI in my localhost beyond resizing the browser, so I'll only say it should fix those issues. Please test it and let me know if you see any problems with the updated CSS. One thing that I did do that might jump out right away is change the dashicon so it uses dashicons-dismiss, which IMO looks a lot better.

@ryan
9 years ago

31233.4.diff, iPhone 6+

#20 @ryan
9 years ago

Attached a fresh mobile screenshot and posted to make/core about mobile patch testing.

https://make.wordpress.org/core/2015/03/16/mobile-patch-testing-with-vvv-and-xip-io/

@valendesigns
9 years ago

@valendesigns
9 years ago

@valendesigns
9 years ago

#21 @valendesigns
9 years ago

The latest patch 31233.5.diff solved the mobile issue. However, disregard it as there are now desktop browser issues with FF and Opera that I discover right after uploading, Chrome & Safari are both playing nice. I'll add a new patch shortly with additional browser testing. Getting closer though.

@valendesigns
9 years ago

Fixes FireFox

#22 @valendesigns
9 years ago

Patch 31233.6.diff fixes FireFox. I also tested IE 8 in WinXP & IE 10 in Win8 and everything looks good. The issue with Opera is that the dashicon's vertical alignment seems to be pushed past the top of the element and none of the alternative attribute values will fix the issue. Not sure if we even support Opera or not, if we don't then I believe this is done.

#23 @adamsilverstein
9 years ago

Tested on Mac, Chrome & Firefox latest release versions and this works well, nice work @valendesigns!

Question: is 'dismiss' too generic a class name? Worried about potential conflicts with themes or plugins, what about something more descriptive: wp-dismissable?

In 31233.7.diff I added a bit of whitespace to the JS and slightly updated selector for click.

@valendesigns
9 years ago

#24 follow-up: @valendesigns
9 years ago

@adamsilverstein Thank you for the patch, though you probably should have tested that it worked because there are JS errors in it. As well, the event was correct the way I had it. You moved the namespace wp-dismiss-notice into the selector, which would also have made it not work.

I added a bit more specificity to the JS by adding .wrap in patch 31233.8.diff and included your whitespace.

The CSS class name .dismiss is already used in other places throughout the Core. I'm not against changing it, but would like to hear what others have to say on the subject.

#25 in reply to: ↑ 24 @adamsilverstein
9 years ago

Darn! I thought I did test and jshint before submitting, although I will admit it was late and I see the typo now that its morning :) I must have been testing your patch... Apologies for breaking anything! I want to see this succeed, went to test and then noticed the whitespace issues I fixed.

New patch looks good.

Replying to valendesigns:

@adamsilverstein Thank you for the patch, though you probably should have tested that it worked because there are JS errors in it. As well, the event was correct the way I had it. You moved the namespace wp-dismiss-notice into the selector, which would also have made it not work.

I added a bit more specificity to the JS by adding .wrap in patch 31233.8.diff and included your whitespace.

The CSS class name .dismiss is already used in other places throughout the Core. I'm not against changing it, but would like to hear what others have to say on the subject.

This ticket was mentioned in Slack in #core by valendesigns. View the logs.


9 years ago

@valendesigns
9 years ago

#27 @valendesigns
9 years ago

  • Keywords make-flow removed

The latest patch 31233.11.diff manually inserts the markup into the notice, which avoids any delay in rending the button. The JS will still automatically append the markup if it's missing from the notice so it can be used in other places in the future without having to manually add it. After some discussion on Slack we decided to switch to slideUp instead of fadeOut as the animation seemed jarring.

Last edited 9 years ago by valendesigns (previous) (diff)

#28 @mordauk
9 years ago

I just gave this a quick test and it works really smoothly for me.

#29 @DrewAPicture
9 years ago

  • Keywords commit added

#30 @paulwilde
9 years ago

Using the class .notice-dismiss, or better yet .notice-dismissable would be better as it gives it context and conforms more with the classes which were introduced in [30505]. It also has the benefit of not being too generic so you can easily search through the entire code-base and know that all of the results refer to this functionality.

All of the .updated and .error classes should eventually be updated to use the .notice-* classes inside the admin as well and just keep the CSS there for back-compact. I'll create a patch (for 4.3, as 4.2 is too far into beta I'm guessing?) at some point which does just that unless someone else wants to champion that.

Also another minor gripe, the classes used are overqualified they should simply just be .notice-close instead of div.close. then the CSS can be refactored to be .notice-close instead of .wrap div.dismiss button.close.

Last edited 9 years ago by paulwilde (previous) (diff)

#31 @valendesigns
9 years ago

@paulwilde I was thinking the same thing about the CSS class. I've updated the patch so that it now uses a more conforming .notice-dismissible instead of .dismiss alone.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by valendesigns. View the logs.


9 years ago

@adamsilverstein
9 years ago

add missing comma

#34 @adamsilverstein
9 years ago

Lookin' good, this is a small AND great addition. In 31233.13.diff I added a missing (final line) comma from the array construction in script-loader.php L83.

#36 @afercia
9 years ago

Please consider to add focus style for the dismiss button. In the proposed refreshed patch:

  • added focus style
  • changed padding to horizontally center the icon
  • CSS fix for IE 8
  • reordered CSS properties, see CSS Property Ordering
  • all non-submit button should have a type attribute type="button"

outstanding issues:

  • maybe "notice-dismissible" should be "notice-dismissable"?
  • color contrast ratio #bbb on #fff is too low, I'd say to iterate on this in #31713
  • just as a note: markup from edit-form-advanced.php will have white space between elements (new lines) while JS generated markup won't, this may cause issues with styling (when/if using inline-blocks)

https://cldup.com/t59FkbUHLx.png

@afercia
9 years ago

@paulwilde
9 years ago

#37 @paulwilde
9 years ago

Attached a patch which further iterates the CSS with the following:

  • Use the class .notice instead of .updated for the notice
  • Use .notice-close instead of deep nesting the CSS with .notice-dimissible button.close

Replying to afercia:

  • maybe "notice-dismissible" should be "notice-dismissable"?

The most popular CSS framework Bootstrap uses "dismissible" so makes most sense to opt for the most widely used/known spelling unless there's any WordPress specific reason to go with dismissable.

#38 @valendesigns
9 years ago

I am currently working on an updated patch to address some of these points you guys have brought up, but the changes in 31233.14.diff break in mobile. Although everyones help is greatly appreciated, at this late stage if you have something you want to add to the discussion please leave a comment and give me time to update the patch before creating your own. When styles are changed the screenshots no longer apply and we end up back at the beginning needing to test all over again. A new patch that has been properly tested will be uploaded shortly. Thank you Guys!

@valendesigns
9 years ago

@valendesigns
9 years ago

@valendesigns
9 years ago

@valendesigns
9 years ago

#39 @valendesigns
9 years ago

I've added patch 31233.16.diff which combines some of the changes from the recent patches and expands on them. As well, I've attached screenshots for various browsers and devices. I tested in Chrome, Firefox, Safari, & Opera on OSX and IE 8-10 on Windows 8, and iPad 4 & iPhone 5. Unless there is something that's broken, I don't feel this patch should be iterated on. We are getting way to close to 4.2 to be making anymore adjustments. Please test and respond with your findings so we can get this in the Core. Cheers!

@valendesigns
9 years ago

fix whitespace

@valendesigns
9 years ago

Forgot type="button"

#40 follow-up: @valendesigns
9 years ago

Now that we have a proper focus state, I think we should go back to fadeOut because slideUp looks pretty bad in some browsers since part of the outline gets cutoff while it's animating. I think the only browser that really looks best with slideUp is Firefox. I would prefer fadeOut in all other browsers when a focus style is applied. Without a focus style I would stick with slideUp. Thoughts?

#41 in reply to: ↑ 40 ; follow-up: @DrewAPicture
9 years ago

Replying to valendesigns:

Now that we have a proper focus state, I think we should go back to fadeOut because slideUp looks pretty bad in some browsers since part of the outline gets cutoff while it's animating. I think the only browser that really looks best with slideUp is Firefox. I would prefer fadeOut in all other browsers when a focus style is applied. Without a focus style I would stick with slideUp. Thoughts?

The issue with the fadeOut() effect, as pointed out previously, is that after it fades, everything below it (see: 95 percent of the page markup) jumps up to fill the space. It doesn't need be slideUp(), but whatever it is, it shouldn't be jarring.

... looks pretty bad in some browsers since part of the outline gets cutoff while it's animating.

Not sure I'm following. Are you saying that the focus outline gets cut off while it's animating? If so, I guess I'm not sure why that matters, it's in the process of being animated from visible to hidden.

#42 in reply to: ↑ 41 @afercia
9 years ago

Replying to DrewAPicture:

the focus outline gets cut off while it's animating?

slideUp() applies overflow: hidden on the element being animated, see screenshot:

https://cldup.com/GFwexXmrIZ.png

was not happening before because the dismiss button was a bit smaller:

https://cldup.com/g7HDM6hm1q.png

I'd say this is an opportunity to solve two issues at the same time: when the dismiss button is focused and you activate it with your keyboard, there's a focus loss.
As far as I know, Firefox is the only browser that keeps focus "in place" but with all other browsers focus will fallback to the document and you will have to start tabbing from the beginning of the page.

Simply moving the focus to a proper place when the dismiss button is activated would solve both issues. Would propose to move it to the main content div #wpbody-content which is focusable thanks to tabindex=0.

Not for arguing, but please notice in the latest patch there are a couple of trailing spaces and one tab character to remove.

Also I'm not sure noindex:-o-prefocus is really needed, @valendesigns could you please expand on that?

#43 @valendesigns
9 years ago

Replying to DrewAPicture:

The issue with the fadeOut() effect, as pointed out previously, is that after it fades, everything below it (see: 95 percent of the page markup) jumps up to fill the space. It doesn't need be slideUp(), but whatever it is, it shouldn't be jarring.

I've fixed it with some modification in 31233.19.diff using fadeTo then slideUp. It basically does exactly what it sounds like, it fades to transparent without any page movement then slides up. Best of both worlds.

Replying to afercia:

Simply moving the focus to a proper place when the dismiss button is activated would solve both issues. Would propose to move it to the main content div #wpbody-content which is focusable thanks to tabindex=0.

I like the idea in theory. Though in practice adding $( '#wpbody-content' ).focus(); causes the page to jump, which is very jarring. So I would vote against it from a UX stand point.

Not for arguing, but please notice in the latest patch there are a couple of trailing spaces and one tab character to remove.

I found the extra whitespace and tabs in common.css. Thanks for the heads up.

Also I'm not sure noindex:-o-prefocus is really needed, @valendesigns could you please expand on that?

I removed the ones that were related to height & width, those did not need to be separate from the rest of the styles. However, there is one in the mobile styles that must stay because without it Opera does not match the other browsers. The icon is shifted out of alignment by like 10 pixels. With the current CSS there is consistency across a lot of browsers, yes there are a few tricks to get us there but we have to support them. I spent hours testing browser and device support for this patch and unless you can propose alternative CSS to achieve the same level of support I don't see another solution coming easy or without targeting the worst offender, which is Opera. The latest patch does minimize the trickery though now that two of those hacks have been removed.

#44 @helen
9 years ago

  • Keywords dev-feedback commit removed

Some thoughts:

  • Let's use a class of .is-dismissible and target .notice.is-dismissible in JS as well as the one CSS rule. .is-dissmisible itself should likely never have any CSS defined.
  • If we are inserting the button with JS after all, do we really need to put it in PHP for the edit screen notice? I think it's fine to add it with JS and just add the class in PHP.
  • I am not too worried about things just jumping up, given that I think your visual focus would be on the notice. Have not yet tested the animation, though, so we will see how that feels.
  • Should we remove the notice from the DOM once it's hidden?
  • Is removing the .updated class going to affect anything that's assuming that class (plugins/themes doing JS targeting)? Not a blocker, but I don't like to cause breakage without knowing about it first.
  • That Opera hack makes me pretty unhappy to see - which versions of Opera are we talking about? If it's an absolute must, it needs to be commented.
  • All CSS selectors on separate lines, please - if we're touching a line that breaks that rule, then we can fix it here.

#45 follow-up: @DrewAPicture
9 years ago

This will need a new patch incorporating feedback from comment:44 very soon or we'll need to punt to a future release. That doesn't mean progress need stop, we just need to be realistic about forward progress in the 4.2 cycle.

#46 in reply to: ↑ 45 @valendesigns
9 years ago

Replying to DrewAPicture:

This will need a new patch incorporating feedback from comment:44 very soon or we'll need to punt to a future release. That doesn't mean progress need stop, we just need to be realistic about forward progress in the 4.2 cycle.

I'll work on a patch now. Cheers!

@valendesigns
9 years ago

fix typo

#47 @valendesigns
9 years ago

Patch 31233.21.diff is ready for testing.

Replying to helen:

Let's use a class of .is-dismissible and target .notice.is-dismissible in JS as well as the one CSS rule. .is-dissmisible itself should likely never have any CSS defined.

I've switched to .is-dismissible.

If we are inserting the button with JS after all, do we really need to put it in PHP for the edit screen notice? I think it's fine to add it with JS and just add the class in PHP.

I've reverted back to having the JS insert the button for now. We can always add it to the PHP later.

Should we remove the notice from the DOM once it's hidden?

I don't think it would hurt and have added it back to the JS.

Is removing the .updated class going to affect anything that's assuming that class (plugins/themes doing JS targeting)? Not a blocker, but I don't like to cause breakage without knowing about it first.

I don't know if there is an actual concern here. However, so we can move forward without worrying I've made it so the .updated class is still there for backwards compatibility and added the new classes in addition to it so we can use .notice as the parent target.

That Opera hack makes me pretty unhappy to see - which versions of Opera are we talking about? If it's an absolute must, it needs to be commented.

The issue is in Opera 12.16 but I've removed that code and reworked the CSS slightly so that IE is still supported. Opera is the only browser that looks off. It's shifted up by 3-4 pixels. However, most of the other font icons are off so I guess this is acceptable behavior?

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#49 @DrewAPicture
9 years ago

  • Owner set to helen
  • Status changed from new to reviewing

#50 @DrewAPicture
9 years ago

  • Keywords 4.2-strings added

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#52 @helen
9 years ago

  • Keywords changed from has-patch, 4.2-strings to has-patch 4.2-strings

@valendesigns: All looks really good, thanks. In doing final review, etc., I changed .notice-close to .notice-dismiss for consistency and defined the icon for that element rather than keeping the extra span. I also expanded the usage to other places in the admin, of which there were a lot. Most core notices will now have them, with the exception of a couple of errors. There may be some that don't really need to be dismissed, but it's not super intrusive so I'm going to roll with it as-is and we can selectively back stuff out later if something seems really egregious.

Also, I removed the Mozilla hack as well, and defined the padding in px since the button is sized in px. I think there were rounding issues, as it looks good to me now. Again, we can pixel-poke after this commit.

Last edited 9 years ago by helen (previous) (diff)

#53 @helen
9 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 31973:

Admin notices: Make (most) core notices dismissible.

These no longer return upon refreshing the page when JS is on and working, so users should be able to dismiss them. This is particularly important on the post edit screen when DFW is triggered, but pretty much all notices can be dismissed if needed. A post on Make/Core will follow with information on how this can be leveraged in plugins.

props valendesigns, afercia, paulwilde, adamsilverstein, helen.
fixes #31233. see #23367.

#54 @valendesigns
9 years ago

@helen Thank you for getting this into 4.2 before it was too late! As well, adding it to other notices in the Core makes it even that much better. There were some changes you made to the structure & CSS that's causing UI issues though, but I'll work on an updated patch to fix that. Basically in all browsers but Firefox, including iOS devices, the icon is aligned to the right of the 20px container making it look off center when you click on it. I'll see if there is a simple fix and get back to you. Should we reopen the ticket while I adjust the CSS?

#55 follow-up: @valendesigns
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@helen I've uploaded patch 31233.21.2.diff, which returns the UI back to the way things were in 31233.21.diff and without any browser hacks.

  • I added !important to the right padding because text was being overlapped by the dismiss button.
  • By changing the notice paragraph margin from 0.5em to 0.499em I was able to solve the issue with Firefox rounding up an extra pixel from 38 to 39, and without effecting other browsers or devices.
  • The width and height could be removed on the button.
  • I corrected the padding.
  • Moved the button position right: 1px so the highlight lines up with the right edge, which looks better.
  • Added a separate .notice-dismiss:before which corrects the font size and keeps the button from shifting in IE when you click it.

#56 in reply to: ↑ 55 ; follow-up: @helen
9 years ago

Replying to valendesigns:

  • I added !important to the right padding because text was being overlapped by the dismiss button.

Why exactly does that need to be !important? If it really does need to be (I sort of hope not), then it needs to be documented in the CSS.

  • By changing the notice paragraph margin from 0.5em to 0.499em I was able to solve the issue with Firefox rounding up an extra pixel from 38 to 39, and without effecting other browsers or devices.

If we're relying on pixels here, then let's define pixels.

Also - let's remove the border glow on :active. No need for clicking to show that, it's meant as a focus style.

Last edited 9 years ago by helen (previous) (diff)

#57 @DrewAPicture
9 years ago

  • Keywords 4.2-strings removed

The string changes in here are done as of [31973].

#58 in reply to: ↑ 56 @valendesigns
9 years ago

Replying to helen:

Why exactly does that need to be !important? If it really does need to be (I sort of hope not), then it needs to be documented in the CSS.

It's a specificity and/or cascade issue. I would need to dive into it more to say which one it is for certain. But the gist of the issue is that without !important the right padding is not honored.

If we're relying on pixels here, then let's define pixels.

We could do that, but may need to add mobile styles since the font size is different. If you're cool with switching to px I'll add it to the next patch.

Also - let's remove the border glow on :active. No need for clicking to show that, it's meant as a focus style.

Will do!

#59 @valendesigns
9 years ago

@helen I've uploaded patch 31233.22.diff to address your concerns.

I've removed the change I made to the paragraph margins for now, as it's really only an issue in Firefox because the notice height is rounded up to 39px where all the other browsers set it to 38px based on the combined heights of the contents, plus padding and margin. As well, you only notice the height difference when the focus state is on and since we've removed :active Firefox no longer shows that glow when you click the button. It's really not that critical, but to change the .5em margin equivalent to pixels means it would need to be 6.5px and that still displays the same issue in Firefox. I say we revisit it later if it really bothers people. For now we just need to get the button looking fairly consistent.

To remove !important I targeted the notice with .wrapwhich adds more specificity. Hope that works for you?

#60 @DrewAPicture
9 years ago

  • Keywords commit added

@miqrogroove
9 years ago

Feature Bugged in Beta 4 :(

#61 follow-up: @miqrogroove
9 years ago

Seems broken in Beta 4 (4.2-beta4-32051). See attachment.

#62 @miqrogroove
9 years ago

According to Chrome dev tools:

.screen-reader-text {
  position: absolute;

That is causing a mangled button to appear.

#63 in reply to: ↑ 61 @helen
9 years ago

Replying to miqrogroove:

Seems broken in Beta 4 (4.2-beta4-32051). See attachment.

Can't reproduce this. Surely this would be an issue in multiple places in the admin already, as .screen-reader-text is a generic class.

@valendesigns
9 years ago

refresh

@valendesigns
9 years ago

removed whitespace added during copy & paste

#64 follow-up: @valendesigns
9 years ago

@helen I've refreshed the patch, as it wasn't applying cleanly anymore since the color was changed in .notice-dismiss in a different ticket. As for the issues that @miqrogroove is having, to me it looks like your browser has cached the old CSS and you should do a hard refresh to verify.

#65 in reply to: ↑ 64 @miqrogroove
9 years ago

Replying to valendesigns:

As for the issues that @miqrogroove is having, to me it looks like your browser has cached the old CSS and you should do a hard refresh to verify.

Confirmed, I could not produce the same problem after restarting Chrome. Thanks.

#66 @helen
9 years ago

  • Keywords commit removed

@valendesigns A couple more things:

  1. I would use .wp-core-ui instead of .wrap, as it was specifically created as a scoping class.
  2. The icon needs to stay consistent with other usages, size-wise. I know it's slightly different because it doesn't have body text next to it, but it's essentially the same context as the welcome panel dismiss icon. I requested feedback from @melchoyce, who's one of the driving forces behind Dashicons, and she came to the same conclusion. The one place it appears to have the ideal Dashicons size of 20px is the multisite storage limit warning, which has a different color and a very different meaning. We will look at creating a smaller version of the icon so we don't have scaling issues (cough, Windows), but not for 4.2.

#67 @valendesigns
9 years ago

@helen I've updated the patch to use the suggested scoping class and default font size. I don't feel the icon looks as good smaller, but I'm happy to fix it to your spec.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


9 years ago

#69 @helen
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 32068:

Dismissible notices: more precise positioning across browsers.

props valendesigns.
fixes #31233.

Note: See TracTickets for help on using tickets.