Opened 3 years ago
Closed 2 years ago
#54624 closed defect (bug) (fixed)
"Automated update failed" notice breaks Site Health styling
Reported by: | johnjamesjacoby | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | 5.2 |
Component: | Upgrade/Install | Keywords: | has-screenshots has-patch has-testing-info commit |
Focuses: | accessibility, css, administration | Cc: |
Description
When an automated update fails, a notice is shown in WordPress Admin.
This notice appears in such a place and in a way as to make the "Site Health" page header area appear unintentionally offset by it.
Screenshot imminent.
Attachments (10)
Change History (31)
#2
@
2 years ago
- Focuses css added
- Keywords has-patch has-testing-info needs-testing added; needs-patch removed
Testing Instructions
These steps define how to reproduce the issue, and indicate the expected behavior.
Steps to Reproduce
- Create and activate the plugin shown in this comment.
- 🐞 Navigate to Tools > Site Health. The notice appears above the header.
- 🐞 Navigate to Settings > Privacy. The notice appears above the header.
Expected Results
- ❌ Tools > Site Health: The notice appears above the header.
- ❌ Settings > Privacy: The notice appears above the header.
Steps to Test patch 54624.diff
- Run
grunt patch:54624
. - Run
npm run build:dev
(or whichever build script your workflow uses). - Create and activate the plugin shown in this comment.
- Navigate to Dashboard. Ensure that the notice has its typical location and appearance.
- Navigate to Tools > Site Health. The notice should appear below the header and be full width.
- If it does not, perform a hard refresh to clear your cache before posting your test results.
- Navigate to Settings > Privacy. The notice should appear below the header and be full width.
- If it does not, perform a hard refresh to clear your cache before posting your test results.
- Feel free to test in responsive mode for additional test data.
Expected Results
- ✅ Dashboard: The notice has its typical location and appearance.
- ✅ Tools > Site Health: The notice should appear below the header, and be full width.
- ✅ Settings > Privacy: The notice should appear below the header, and be full width.
- ✅ Responsive mode: The notice should appear below the header, and be full width.
Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.
#3
@
2 years ago
- Keywords 2nd-opinion added
@Clorith, if you have time, could you take a look at this video showing patch 54624.diff, patch 54624.diff and let me know your thoughts on this solution?
As an aside, when speaking with @TimothyBlynJacobs, we thought it might make more sense for all notices on these screens to be constrained to the max content width. This would be another ticket, but food for thought.
#4
follow-up:
↓ 5
@
2 years ago
This is actually the case for any notice displayed using the admin_notice
hook, including those from plugins or themes.
It was brought up during the implementation of the Site Health Checker module that these dashboard nags would push it down, I was trying to find that discussion for reference without much luck, but I seem to recall the consensus, at that time, was that it should be left alone until a proper notification system was in place in core instead.
I can't remember the rationale for it without finding the ticket, but I can say that there is a case to be made that anything that goes under the heading becomes associated with that page, and not the WordPress install as a whole, if that's a "good" reason, as opposed to maintaining a nice visual appearance, is another topic :)
#5
in reply to:
↑ 4
@
2 years ago
Replying to Clorith:
It was brought up during the implementation of the Site Health Checker module that these dashboard nags would push it down, I was trying to find that discussion for reference without much luck, but I seem to recall the consensus, at that time, was that it should be left alone until a proper notification system was in place in core instead.
#46651 seems to have a related discussion.
#6
@
2 years ago
My reading of #46651 is that while it wasn't ideal to put notices below the header, after accessibility and design feedback, it was the most suitable available solution.
comment 23 says that the update nag "can't be moved". The current patch provides a way for us to do that.
The Notifications feature project is going out for feedback/testing and not sure when it'll land unfortunately. Until then, I think we can make this change to improve accessibility and have a consistent location for notices on the Site Health and Privacy screens.
This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.
2 years ago
#8
@
2 years ago
- Focuses accessibility added
Pinging @afercia and @joedolson for their input as the order of markup pertains to accessibility and thoughts on the patch, if they have time. 🙂
#9
@
2 years ago
- Keywords needs-testing removed
@costdev while it does look better with your patch, I think the update nag is misplaced. It should ideally be displayed on the top of the screen, and if possible in the white section of the header banner. For consistency with other screens but also because semantically it is not the content of the screen, so it shouldn't appear after the screen heading.
@
2 years ago
Alternative: Wrap the notices in a white container and adjust margin for consistency with other screens.
#10
@
2 years ago
- Keywords needs-testing added
Thanks for the feedback @audrasjb! Can you take a look at 54624.2.diff and see if you think this works better?
#11
@
2 years ago
54624.2.diff looks OK to me visually, but adding a hardcoded list of screens in wp-admin/admin-header.php
does not seem ideal for future maintenance.
Personally, I would like to correct the margins, but keep the grey background, I think that makes the notices more readable and more in line with other admin screens. We should also test this with multiple admin notices displayed at the same time.
54624.3.diff would be my suggestion here, see 54624.3.png.
#12
@
2 years ago
I also think it's easier to keep the notices on the very top of admin screen, before the white banner, and it's also easier to maintain since it's just a CSS tweak.
This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.
2 years ago
#14
@
2 years ago
Consistency is pretty important from an accessibility perspective, so I think it would be better if the order isn't changed for design reasons on a few isolated pages.
#15
@
2 years ago
it's easier to keep the notices on the very top of admin screen, before the white banner, and it's also easier to maintain since it's just a CSS tweak.
Consistency is pretty important from an accessibility perspective
+1 to both, from me!
Visually, what @SergeyBiryukov suggested is not pleasing 😅 but I don't dislike it for the here & now 💗
This is me yearning for an official style-guide (cc @helen & @bueltge 💪) for the various ways that admin-area UI elements are intended to be used – and potentially how they should not be. UI like Chonky Header were not designed with the intention of anything appearing above it – not even Notices – and a semi-formal collision assessment on it would've likely revealed Notices as a blocker (cc @afercia 💪)
#16
@
2 years ago
- Keywords 2nd-opinion needs-testing removed
Just tested 54624.3.diff and it looks the same as shown in 54624.3.png and looks as expected on mobile.
As we have four committers in favour of 54624.3.diff, it looks like this one's ready for commit
consideration.
Any takers? 🙂
#17
@
2 years ago
- Keywords commit added
- Owner set to audrasjb
- Status changed from new to accepted
Alright! I'll ship this :)
Thanks!
Self assigning for commit
.
A few notes on this:
inline
class are placed below the header.maintenance_nag()
,update_nag()
and for multisite,site_admin_notice()
all have theinline
class. A solution could be to remove this class to create a consistent display.