Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#37504 closed defect (bug) (fixed)

Update message displays unstyled in plugins list

Reported by: ovann86's profile ovann86 Owned by: drewapicture's profile DrewAPicture
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 SergeyBiryukov)

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)

WP45x.png (88.2 KB) - added by ovann86 9 years ago.
WordPress 4.5.x - Installed Plugins page with update message correctly styled
WP46x.png (78.6 KB) - added by ovann86 9 years ago.
WordPress 4.6-RC1-38170 - Installed Plugins page with update message unstyled
itsg-demo-update-message.zip (781 bytes) - added by ovann86 9 years ago.
Plugin to demonstrate a plugin update message
37504.patch (2.3 KB) - added by rahulsprajapati 9 years ago.
Added css for WP < 4.6 version update message displays using :not(.notice) as .notice class is added after 4.6
37504.PNG (63.3 KB) - added by Clorith 9 years ago.
37504.1.patch (2.2 KB) - added by rahulsprajapati 9 years ago.
Refreshed Patch : Fixed box-shadow as mention in comment:8 by @Clorith
update-messages-45.png (295.8 KB) - added by ocean90 9 years ago.
update-messages-46.png (269.6 KB) - added by ocean90 9 years ago.
update-messages-46-patched.png (280.3 KB) - added by ocean90 9 years ago.
37504.2.patch (2.2 KB) - added by ocean90 9 years ago.
37504.3.patch (2.3 KB) - added by ocean90 9 years ago.
37504.4.patch (2.3 KB) - added by ocean90 9 years ago.
37504.5.patch (1.3 KB) - added by ocean90 9 years ago.
Without icon: https://cloudup.com/cfmJHMcJLFA
37504.diff (842 bytes) - added by DrewAPicture 9 years ago.
37504diff_inactive.png (48.9 KB) - added by DrewAPicture 9 years ago.
w/ 37504.diff
37504diff_active_no_class.png (49.3 KB) - added by DrewAPicture 9 years ago.
w/ 37504.diff
37504_active_with_class.png (50.4 KB) - added by DrewAPicture 9 years ago.
w/ 37504.diff

Download all attachments as: .zip

Change History (52)

@ovann86
9 years ago

WordPress 4.5.x - Installed Plugins page with update message correctly styled

@ovann86
9 years ago

WordPress 4.6-RC1-38170 - Installed Plugins page with update message unstyled

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


9 years ago

@ovann86
9 years ago

Plugin to demonstrate a plugin update message

#2 @SergeyBiryukov
9 years ago

  • Component changed from Administration to Plugins
  • Description modified (diff)
  • Focuses administration added; template removed

#3 @jorbin
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.6

#4 follow-up: @afragen
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

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

#5 in reply to: ↑ 4 @Clorith
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.

@rahulsprajapati
9 years ago

Added css for WP < 4.6 version update message displays using :not(.notice) as .notice class is added after 4.6

#6 @rahulsprajapati
9 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.


9 years ago

@Clorith
9 years ago

#8 follow-up: @Clorith
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.

@rahulsprajapati
9 years ago

Refreshed Patch : Fixed box-shadow as mention in comment:8 by @Clorith

#9 in reply to: ↑ 8 @rahulsprajapati
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 @obenland
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 @Clorith
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

#13 @DrewAPicture
9 years ago

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

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


9 years ago

#15 @DrewAPicture
9 years ago

  • Keywords commit added

37504.1.patch looks good and works well for me.

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


9 years ago

#17 follow-up: @helen
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 @rahulsprajapati
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. :)

@ocean90
9 years ago

#19 @ocean90
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

@ocean90
9 years ago

#20 @ocean90
9 years ago

37504.3.patch:

  • 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.
Last edited 9 years ago by ocean90 (previous) (diff)

@ocean90
9 years ago

#21 in reply to: ↑ 17 ; follow-up: @ocean90
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 @afragen
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 @SergeyBiryukov
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 @swissspidy
9 years ago

@SergeyBiryukov Because that would address div.update-message.notice too, which is not wanted here.

#26 @SergeyBiryukov
9 years ago

Thanks, looks good to me then.

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


9 years ago

#28 @DrewAPicture
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 @afragen
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 @DrewAPicture
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.

@DrewAPicture
9 years ago

#31 @afragen
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 @DrewAPicture
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.

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

#33 @afragen
9 years ago

Added 37504.diff​, which restores the padding and margin only for the update-message class.

I like this.

#34 @DrewAPicture
9 years ago

In 38237:

Update/Install: Provide basic back-compat styling for the .update-message CSS class in the plugins list table.

This change restores only the margin and padding styles for the .update-message class when used by plugins in the context of adding arbitrary rows to the list table. The inline-update colors and icon styles were not restored, expressly with a wide variety of plugin use-cases in mind.

Props ovann86, rahulsprajapati, ocean90, DrewAPicture.
Props helen for review.
See #37504.

#35 @DrewAPicture
9 years ago

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

In 38238:

Update/Install: Provide basic back-compat styling for the .update-message CSS class in the plugins list table.

This change restores only the margin and padding styles for the .update-message class when used by plugins in the context of adding arbitrary rows to the list table. The inline-update colors and icon styles were not restored, expressly with a wide variety of plugin use-cases in mind.

Merge of [38237] to the 4.6 branch.

Props ovann86, rahulsprajapati, ocean90, DrewAPicture.
Props helen for review.
Fixes #37504.

Note: See TracTickets for help on using tickets.