Opened 9 years ago
Closed 9 years ago
#37504 closed defect (bug) (fixed)
Update message displays unstyled in plugins list
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.6 | Priority: | normal |
Severity: | normal | Version: | 4.6 |
Component: | Plugins | Keywords: | has-patch has-screenshots commit |
Focuses: | ui, administration | Cc: |
Description (last modified by )
Appears to have occurred after changeset r37714
https://core.trac.wordpress.org/changeset/37714/trunk/src/wp-admin/css/list-tables.css
When a plugin uses an update message, for example a registration reminder, some basic styling is applied to make it stand out - background red, padding etc.
In 4.6-RC1-38170 the styles aren't applied and are missing from /wp-admin/css/list-tables.css
Attachments (17)
Change History (52)
This ticket was mentioned in Slack in #core by ovann86. View the logs.
9 years ago
#2
@
9 years ago
- Component changed from Administration to Plugins
- Description modified (diff)
- Focuses administration added; template removed
#4
follow-up:
↓ 5
@
9 years ago
Shiny Updates has merely required some changes to the class tags that go into the after plugin row data. I think it may also require the plugin to wrap the message in a p tag also.
I'm not sure why the plugin dev is not responsible for updating their code. Isn't this exactly why we release betas?
This is how I did it in a plugin, https://github.com/afragen/github-updater/blob/develop/src/GitHub_Updater/Base.php#L1024-L1055
#5
in reply to:
↑ 4
@
9 years ago
I had a quick look, it's a simple process to re-introducing everything with the exception of the spinner icon.
Previously we attached the icon to .update-message:before
, but we now target .update-message p:before
instead.
Re-introduction with the spinner would mean that shiny updates receives two of them, so this might require some output changes to shiny updates output as well. Alternatively we could omit the icon and just attach the other styles so it doesn't look like something is broken?
Replying to afragen:
I'm not sure why the plugin dev is not responsible for updating their code. Isn't this exactly why we release betas?
It is, but there's quite a few plugins that utilizes this approach, and expecting them all to have sorted it is (unfortuantely) unrealistic, and having a somewhat broken UI isn't great for the user, and the user should always be our priority.
Normally if we introduce things that could break how things are perceived, we'd have included it in one of the posts leading up to the release though, but the CSS changes haven't been mentioned so many authors may be blissfully unaware of this change.
@
9 years ago
Added css for WP < 4.6 version update message displays using :not(.notice) as .notice class is added after 4.6
This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.
9 years ago
#8
follow-up:
↓ 9
@
9 years ago
37504.patch sort of works, but there's a visual separation, you may want to have a quick look at list-tables.css line 1294 which covers removing box-shadows of the item that owns the update line, the attached screenshot covers the patched view first, then shiny updates.
Not sure how much it matters, but the box shadow does make it feel like there's meant to be plugin data that is missing inside that table row.
#9
in reply to:
↑ 8
@
9 years ago
Replying to Clorith:
37504.patch sort of works, but there's a visual separation, you may want to have a quick look at list-tables.css line 1294 which covers removing box-shadows of the item that owns the update line, the attached screenshot covers the patched view first, then shiny updates.
Not sure how much it matters, but the box shadow does make it feel like there's meant to be plugin data that is missing inside that table row.
Please check 37504.1.patch.
#10
@
9 years ago
Right, the parent tr needs an .update
class to remove the box shadow, but that hasn't changed from 4.5.
I think the latest patch achieves what we're looking for in terms of backwards compatibility (although the box shadow bits can be removed). It still makes for a poor user experience in my opinion when core elements are hijacked like this. If there is an update to a plugin adding a custom notification it will result in two notifications being shown. And if it doesn't pretend to have an update available, the notice hangs in between two plugin rows, visually not belonging to either really, while sporting an update dashicon for a non-update message. See 37504.PNG.
#11
@
9 years ago
Patch looks good for backwards compatibility, there's not much more we can (or should) do with this one.
Ideally those who use the update area for other messages should move them over to admin notices instead, and hopefully will after 4.6.
This ticket was mentioned in Slack in #core by ocean90. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by drew. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by drew. View the logs.
9 years ago
#17
follow-up:
↓ 21
@
9 years ago
- Keywords commit removed
I'm not sure I understand why the :not(.notice)
selectors are necessary. Also, since there are different notice styles, please test and make sure that the colors can still be overridden by those classes (e.g. .notice-info
).
#18
@
9 years ago
@helen
I have used :not(.notice)
selector because of this markup we are using in update message from 4.6
WP 4.6
:
<div class="update-message notice inline notice-warning notice-alt"> <p>Update Message..</p> </div>
WP < 4.6
:
<div class="update-message"> Update Message.. </div>
Now, here I can see only one way for backwards compatibility, using :not(.notice)
selector. I did check this .update-message
class in the core and it's not used in other than update message sections. So I think .update-message:not(.notice)
should not conflict with any other CSS of the core.
We can also use notice-warning
class for :not
selector.
Thank you. :)
#19
@
9 years ago
- Keywords has-screenshots added
37504.2.patch is a refresh of 37504.1.patch:
- Replace 6.5px with 0.5em
- One selector per line
- Merge the styles for
.notice
and.update-message
- Adds a
/* back-compat for pre-4.6 */
comment - Removes a stray comma
An alternative would be to replace all .update-message
classes with a new class for the new styles and restore the old styles as .update-message
#20
@
9 years ago
- Removes the
:not(.notice)
selector from.plugins .plugin-update-tr .update-message:not(.notice)
..plugins .plugin-update-tr .update-message
is the same as.plugins .plugin-update-tr .notice
. - Removes
.update-message:not(.notice)
from the.plugins .notice p
rule. The rule for.plugins .plugin-update-tr .update-message
has already the correct margins. - Merges
.update-message:not(.notice):before
with.wrap .notice p:before
.
#21
in reply to:
↑ 17
;
follow-up:
↓ 24
@
9 years ago
- Keywords commit added
Replying to helen:
I'm not sure I understand why the
:not(.notice)
selectors are necessary. Also, since there are different notice styles, please test and make sure that the colors can still be overridden by those classes (e.g..notice-info
).
HTML markup of a table row for an update in…
…WordPress 4.5:
htm <tr class="plugin-update-tr active" id="hello-dolly-update" data-slug="hello-dolly" data-plugin="hello.php"> <td colspan="3" class="plugin-update colspanchange"> <div class="update-message"> <!-- This element needs to be addressed. --> There is a new version of Hello Dolly available. <a href="" class="thickbox open-plugin-details-modal" aria-label="View Hello Dolly version 1.6 details">View version 1.6 details</a> or <a href="" class="update-link" aria-label="Update Hello Dolly now">update now</a>. </div> </td> </tr>
…trunk:
htm <tr class="plugin-update-tr" id="hello-dolly-update" data-slug="hello-dolly" data-plugin="hello-dolly/hello.php"> <td colspan="3" class="plugin-update colspanchange"> <div class="update-message notice inline notice-warning notice-alt"> <p> There is a new version of Hello Dolly available. <a href="" class="thickbox open-plugin-details-modal" aria-label="View Hello Dolly version 1.6 details">View version 1.6 details</a> or <a href="" class="update-link" aria-label="Update Hello Dolly now">update now</a>. </p> </div> </td> </tr>
By using :not(.notice)
we only address the element which has the .update-message
class, no other notice classes are involved.
To make this clearer we could use another selector: div[class="update-message"]
, see 37504.4.patch.
Proposing 37504.3.patch or 37504.4.patch for commit.
#22
@
9 years ago
Just tossing my novice opinion that 37504.4 appears a cleaner solution.
Also, 4.5 plugin row messages won't have a spinner without the surrounding <p>
tags but that's probably a good thing. Here I'm referring to a message as in @Clorith's first image above.
This ticket was mentioned in Slack in #core by ocean90. View the logs.
9 years ago
#24
in reply to:
↑ 21
@
9 years ago
Replying to ocean90:
To make this clearer we could use another selector:
div[class="update-message"]
, see 37504.4.patch.
Any reason it's not just div.update-message
?
#25
@
9 years ago
@SergeyBiryukov Because that would address div.update-message.notice
too, which is not wanted here.
This ticket was mentioned in Slack in #core by ocean90. View the logs.
9 years ago
#28
@
9 years ago
@ocean90 What would be the purpose behind removing the icon? If the plugin isn't adding their own new row it's going to have the icon anyway.
My +1 is on 37504.4.patch.
#29
@
9 years ago
+1 for 37504.5.patch (without update icon)
Isn't this is ticket about adding an after_plugin_row_{$slug}
for registration or other notices. It doesn't make sense to add the update icon.
@DrewAPicture if the plugin isn't adding their own new row there won't be a difference. If, however, the plugin is adding a new row. Say for a registration notice or anything other that an update notice, there's no reason to have the icon. At least that's how I use it.
#30
@
9 years ago
@afragen See the linked discussion in Slack. I was making the assumption that developers using the update-message
class on a div would be using it for update message purposes.
The other suggestion I made was to simply avoid it looking completely broken by only supporting back-compat on the padding and margin but not the colors/icon at all, i.e. https://cl.ly/1I460V452u3c
In that screenshot, the first one is the row added by the plugin. The second one is added by core.
#31
@
9 years ago
@DrewAPicture sorry I saw the trac message before looking at Slack :P
Unfortunately using .update-message
is the simplest way to add a similarly styled row here. I agree with you idea behind just setting the margin/padding so it doesn't appear broken. It should be on the dev implementing this to fix/style it how they choose. I would be quite happy with just having a margin/padding fix. For a non-update notice in this position the colors/border/icon aren't really needed, but I think the simplest fix here is probably the best.
What bothers me is the extra "border" line on the top of such a non-update notice giving it the appearance that it's somehow separate from the plugin above. This seems to be from a box-shadow
but still would need the bottom of that, just not the top. BTW, this doesn't happen for actual update messages. Perhaps another ticket is needed for this.
#32
@
9 years ago
Added 37504.diff, which restores the padding and margin only for the update-message
class.
This approach is simple, and provides basic styling so it doesn't look as broken, and makes no assumptions about what the added div contains.
Edit: Note: The double notice in the screenshots is due to the plugin only displaying the extra row on subsites in multisite, which apparently were not supported at some point previously. The top one is the notice added by the plugin, the bottom supplied by core.
If the plugin is inactive, looks like 37504diff_inactive.png.
If the plugin is active and the plugin hasn't incorporated the .active
class alongside .plugin-update-tr
on the <tr>
, looks like 37504diff_active_no_class.png.
If the plugin is active and the plugin has incorporated the .active
class, looks like 37504_active_with_class.png.
WordPress 4.5.x - Installed Plugins page with update message correctly styled