#39213 closed defect (bug) (fixed)
Audit the network pages notices.
Reported by: | afercia | Owned by: | joedolson |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | Networks and Sites | Keywords: | has-ui-feedback good-first-bug has-patch commit |
Focuses: | ui, multisite | Cc: |
Description (last modified by )
Some notices and messages in the network pages are printed out in simple paragraphs and don't make use of the WordPress notices styling. For example, in my-sites.php
:
_e( 'You must be a member of at least one site to use this page.' )
There are probably a few ones across the network screens. For design consistency, they should all use a wrapper <div>
element with the proper CSS classes notice
, notice-success
, notice-error
etc. Also, the old CSS classes updated
and error
should be replaced with the new ones, where applicable.
Attachments (8)
Change History (41)
#3
@
8 years ago
- Keywords has-patch added
- Version set to 4.7
Hello, people!
I've researched on this file and found only 2 occurrences:
Line 54 and 71.
Line 71 should be protected by the capabilities, but I've updated it as well. See attached the patch.
#4
@
7 years ago
- Owner set to marksabbath
- Status changed from new to assigned
Assigning ownership to mark the good-first-bug
as "claimed".
This ticket was mentioned in Slack in #core-multisite by stevenkword. View the logs.
7 years ago
#6
@
7 years ago
- Keywords needs-testing added
39213.diff still applies cleanly as of July 10, 2017.
Per multisite bug scrub: " @earnjam -- I don’t see any issues with the patch on 39213. Would just want to confirm there weren’t any other notices/messages that needed styling that were missed."
@marksabbath -- This looks largely complete. What tasks do you feel is missing in order to push this over the finish line? I'm adding the keyword, 'needs-user-testing' for now. Do you need any feedback?
#7
@
7 years ago
- Keywords needs-refresh added
The elements nesting in the second notice in the patch is not correct:
<p><strong> ... </p></strong>
#8
@
7 years ago
You're completely correct @afercia thanks for noticing and reporting that error. It should be fixed in this patch: https://core.trac.wordpress.org/attachment/ticket/39213/39213.1.diff
@stevenkword I think we're good to go. Thank you all for reviewing and sending feedbacks.
#9
@
7 years ago
Hey @stevenkword anything that I could be doing to make this update moving forward?
This ticket was mentioned in Slack in #core-multisite by marksabbath. View the logs.
7 years ago
#11
@
7 years ago
- Keywords needs-testing needs-refresh removed
- Milestone changed from Awaiting Review to 5.0
- Owner changed from marksabbath to jeremyfelt
- Status changed from assigned to reviewing
These changes look good. There's an extra >
in the class attribute on the second message, but I'll take care of that in commit.
It takes messing with capabilities to even see the "You must be a member of at least one site to use this page." message as there are two other errors that would more likely show beforehand, but this is now displayed consistent with other error notices.
Thanks @marksabbath!
#12
@
7 years ago
- Keywords needs-patch added; has-patch removed
It does look like there several other notices that can be updated with notice-error
and notice-success
in src/wp-admin/network/*.php
.
@marksabbath Would you be okay submitting an updated patch covering those as well?
#14
@
6 years ago
Hey @jeremyfelt here is the complete patch: https://core.trac.wordpress.org/attachment/ticket/39213/39213.patch. Would you mind taking a look at it and let me know if there's any other thing that we would need to do to move this forward?
Thanks in advance :)
#16
@
6 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
- Milestone changed from 5.1 to Future Release
Patch needs review.
This ticket was mentioned in Slack in #core-multisite by marksabbath. View the logs.
6 years ago
This ticket was mentioned in Slack in #core by sergey. View the logs.
2 years ago
#20
@
2 years ago
- Keywords needs-refresh added
I tried applying the most recent patch, and it has some conflicts.
#21
@
2 years ago
- Milestone changed from 6.1 to 6.2
This one still needs a refresh and thorough review. I'm going to punt to 6.2 as RC 1 is due out early next week.
This ticket was mentioned in PR #3888 on WordPress/wordpress-develop by @robinwpdeveloper.
23 months ago
#22
- Keywords needs-refresh removed
Trac ticket: https://core.trac.wordpress.org/ticket/39213
#23
@
23 months ago
I have created a new PR.
Followed this patch https://core.trac.wordpress.org/attachment/ticket/39213/39213.patch.
Few things I did different than the last patch:
- Kept
id="message"
- kept
inline
class as it is neitherupdated
nornotice
class - Made few untouched occurrences as well.
Let me know if I need to do any more changes.
#24
@
23 months ago
Tested PR: https://github.com/WordPress/wordpress-develop/pull/3888.
Searched with class="updated notice
in directory src/wp-admin/network/*.php
Before: 18 results in 10 files. [Screenshot https://d.pr/i/k2uoPW ]
After: 0 results. [Screenshot https://d.pr/i/6OwBA9 ]
Let me know how to test the UI or which pages to test.
#26
@
22 months ago
Just added an updated patch based on @robinwpdeveloper's PR. Handles a few additional cases that were missed & uses a variable ID attribute in the one case (user-new.php) where messages can be an array when multiple users are added at once. The first message will still be id="message"
but subsequent will be id="message-{$i}"
.
This ticket was mentioned in PR #4118 on WordPress/wordpress-develop by @joedolson.
22 months ago
#27
Updates classes used to be consistent; prevents duplicate IDs; uses same markup throughout.
Trac ticket: https://core.trac.wordpress.org/ticket/39213
#28
@
22 months ago
- Keywords commit added; dev-feedback removed
- Status changed from reviewing to accepted
This looks ready to go to me; lets get it done.
#29
@
22 months ago
My one hesitation is that I can't see a way that the add new user message ends up as an array with multiple elements, so I'm feeling like it's unnecessary to construct that array rather than just generating a string. There's no iteration present that would add additional messages to the array, so it's always an array with one element.
Adding an alternate version of this that simplifies this code.
@
22 months ago
Alternate version that builds success message for add new user as a string instead of array.
#30
@
22 months ago
If there's any super obscure way that those messages can actually be an array, I think it'll only come up in use. But I don't think there is, so let's get this moving, for real this time.
Good catch, I agree this should have div added for design consistency and the proper class.