Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#50537 closed defect (bug) (fixed)

The buttons colors don't adapt properly to the admin scheme color on WP-Admin

Reported by: youknowriad's profile youknowriad Owned by: youknowriad's profile youknowriad
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.4
Component: Administration Keywords: has-patch
Focuses: accessibility, css Cc:

Description

When you select a deferent admin-scheme, primary buttons change color, but secondary ones don't and the ".page-title-action" buttons don't adapt either.

Attachments (3)

Capture d’écran 2020-07-02 à 5.48.14 PM.png (188.5 KB) - added by youknowriad 4 years ago.
This screenshot shows the result after fixing the issues.
midnight-before.png (190.2 KB) - added by ocean90 4 years ago.
midnight-after.png (185.2 KB) - added by ocean90 4 years ago.

Download all attachments as: .zip

Change History (23)

@youknowriad
4 years ago

This screenshot shows the result after fixing the issues.

#1 @youknowriad
4 years ago

  • Status changed from assigned to accepted

This ticket was mentioned in PR #379 on WordPress/wordpress-develop by youknowriad.


4 years ago
#2

  • Keywords has-patch added

#3 follow-up: @youknowriad
4 years ago

  • Focuses accessibility added
  • Keywords needs-design-feedback added

I'm not sure whether this requires an a11y feedback because it only touches the admin scheme colors and not the default one but I added the label regardless.

#4 in reply to: ↑ 3 @mapk
4 years ago

Replying to youknowriad:

I'm not sure whether this requires an a11y feedback because it only touches the admin scheme colors and not the default one but I added the label regardless.

Because it's only touching an alternate admin theme, I don't think it needs an accessibility review based on this slack comment from @joedolson: https://wordpress.slack.com/archives/C02S78ZAL/p1593629194193300 but I could be wrong.

I haven't tested your patch but the screenshot looks fantastic. I would also expect the primary color to change on the secondary buttons too.

#5 @mapk
4 years ago

  • Keywords needs-design-feedback removed

#6 @youknowriad
4 years ago

I just noticed the primary hover color is broken, I'll try to fix that a bit later.

#7 @michaelarestad
4 years ago

Tested. It looks particularly good for the most part. I also see busted primary button hover broken and some of the schemes are a bit light, but perhaps they need a minor updated.

#8 @Joen
4 years ago

Wonderful small iteration.

I look forward to helping #49999 with improving things overall, there's so much low hanging fruit that just needs a small tweak like this here and there.

#9 @youknowriad
4 years ago

Thanks all of you for the feedback, I fixed the hover issue for primary buttons.

Now about the low contrast in some admin scheme colors, I think the main issue is not from this ticket but I think that the "highlight color" for these admin schemes is just not a good color. The low contrast can be seen in other places too. So I was hesitant on whether to address it here or not.

@ryelle also proposed that we define a new Sass variable for the default/secondary button colors. In that same vein and since I believe the issue is not that the color between secondary and primary should be different, I'm going to define a variable that if set we fallback to the neutral secondary buttons. It's a measure to ensure the color schemes that don't have good colors won't be impacted by this PR while by default all schemes will adapt (just like the default admin scheme).

Last edited 4 years ago by youknowriad (previous) (diff)

#10 follow-up: @youknowriad
4 years ago

I updated the patch and kept the default colors for ocean and coffee themes. Let me know how looks it now. If folks agree it's a good shape, it's still time to ship on 5.5.

#11 @youknowriad
4 years ago

I'd also appreciate a review on https://core.trac.wordpress.org/ticket/50536 (it should be more straightforward as just a simple fix)

Version 0, edited 4 years ago by youknowriad (next)

#12 @youknowriad
4 years ago

Here's another related ticket that I'd appreciate eyes on. #50547

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#13 in reply to: ↑ 10 @mcsf
4 years ago

Replying to youknowriad:

I updated the patch and kept the default colors for ocean and coffee themes. Let me know how looks it now. If folks agree it's a good shape, it's still time to ship on 5.5.

Reviewed, tested, and it looks great.

#14 @ocean90
4 years ago

It looks like the patch reverts parts of #48585 / #49003. Attached two before/after screenshots with the Midnight scheme.

#15 @youknowriad
4 years ago

@ocean90 Yes, that's intended, here's my reasoning for adapting the Midnight scheme buttons to the theme color;

  • We already use the red button for the primary button
  • The color has enough contrast
  • While I understand the "color look like a danger button" argument, it also applies to the primary button right now in trunk, IMO we should be consistent.

That said, it's just a variable change, I'd prefer adapting personally but I don't mind reverting too much.

Last edited 4 years ago by youknowriad (previous) (diff)

#16 @youknowriad
4 years ago

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

In 48342:

Administration: Adapts the secondary buttons colors to the admin schemes.

The admin schemes that don't meet the contrast guidelines are excluded from this change.

Props mapk, michaelarestad, mcsf, ocean90.
Closes #50537.

#17 @youknowriad
4 years ago

I think some colors in the admin schemes need to be rethought, I created this #50575 to continue the discussion.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#18 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5

#19 @ryelle
4 years ago

#48998 was marked as a duplicate.

#20 @ryelle
3 years ago

#48715 was marked as a duplicate.

Note: See TracTickets for help on using tickets.