WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 5 months ago

#38776 closed defect (bug) (fixed)

Twenty Seventeen: Remove color scheme support from edit shortcuts

Reported by: helen Owned by: davidakennedy
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch
Focuses: ui Cc:

Description

As I believe has been discussed, edit shortcuts should remain "WordPress blue" - they are not a part of the theme, so blending in is not a goal, but rather they should be associated with admin/edit UI. To that end, Twenty Seventeen should have the selectors removed from the custom color scheme CSS.

Can't quite recall where/when exactly this has been discussed, so please link tickets/discussions if you know them :)

Attachments (3)

38776.0.diff (1.2 KB) - added by westonruter 6 months ago.
38776.1.diff (1.5 KB) - added by westonruter 6 months ago.
38776.2.remove-2017-rules.diff (1.0 KB) - added by westonruter 5 months ago.

Download all attachments as: .zip

Change History (19)

#1 @helen
6 months ago

@melchoyce: Please correct me if I'm misremembering :)

#2 @melchoyce
6 months ago

@helen You are correct. 👍

@westonruter
6 months ago

#3 @westonruter
6 months ago

  • Keywords has-patch added; needs-patch good-first-bug removed
  • Owner set to sirbrillig
  • Status changed from new to assigned

It's not just the removal of the selector from Twenty Seventeen. It seems the background also needs an !important to prevent the theme's styles from affecting the color.

@sirbrillig Why was there both a background and a background-color? Are there other properties that need !important to ensure themes don't cause the edit shortcuts to appear unexpectedly?

#4 @celloexpressions
6 months ago

There was discussion on this but a consensus was not reached and the Twenty Seventeen implementation was done as an experiment to gather feedback and decide whether themes should be encouraged to style them (it was decided that they should use the core colors on the light and dark schemes in the theme, where there isn't an obvious non-core color to use). The fact that you can't hide the icons while customizing could cause multiple challenges here - on the one hand, it makes the color hue option in Twenty Seventeen both more visible and more confusing (see user testing on make/design). But on the other hand, if the icons severely clash with theme colors the overall UX would be better if themes restyle them, so I don't think we should necessarily discourage that.

The customizer is extremely neutral in its use of colors; the large amounts of blue in the icons diverge from that significantly. If we want to discourage themes from changing the colors, they should be brought more inline with the rest of the customizer design and simultaneously made less likely to clash with theme colors. Gray seems like an obvious choice; perhaps something based on the core secondary button styles?

@westonruter
6 months ago

#5 @westonruter
6 months ago

I can see now that the !important will be needed since #38758 now uses a change to the background-color on hover, and the style rule in Twenty Seventeen is overriding it. See 38776.1.diff which ensures that the edit shortcut button gets expected background color at rest and at hover.

#6 @westonruter
6 months ago

@melchoyce regarding the @celloexpressions's suggestion of using a gray color instead of blue, I seem to recall a mock you had where gray was also used?

#7 @transl8or
6 months ago

@westonruter The gray and square button styles are above that comment:
https://core.trac.wordpress.org/ticket/27403#comment:59

#8 @westonruter
5 months ago

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

In 39243:

Customize: Ensure edit shortcuts have same background color regardless of theme colors.

Fixes #38776.

#9 @westonruter
5 months ago

We can re-open this or create a new ticket if we want to go with gray edit shortcuts.

#10 @sirbrillig
5 months ago

@sirbrillig Why was there both a background and a background-color? Are there other properties that need !important to ensure themes don't cause the edit shortcuts to appear unexpectedly?

Yes, having both properties was to help prevent those properties from being overridden by the theme. In my testing, both were needed, although it looks as though using !important rules takes care of that.

#11 @celloexpressions
5 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Twenty Seventeen still resets the text-shadow when there's a custom color scheme - that needs to be removed (see https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentyseventeen/style.css#L2994). There's another style that's probably not needed, here: https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentyseventeen/style.css#L2984.

Since the icons are persistently on with no way to turn them off while customizing, and we're discouraging themes from recoloring them, I'd definitely prefer using a neutral color. That follows the UI patterns of the customize pane (which uses the blue accent color only very minimally) and will also mitigate clashes with theme colors. Themes (or custom colors) using blue prominently will tend to clash pretty badly with the fairly uncommon hue of WP blue that the icons have here.

#12 @helen
5 months ago

From what I recall, both @melchoyce and I agreed that the clashing was more of a feature than a problem. They're supposed to be noticeable, and while I agree that methods of hiding them beyond hiding all controls need to be explored, I don't think they need to be made to blend in otherwise.

#13 @celloexpressions
5 months ago

In this case, I see a difference between standing out (okay, and ideal) and clashing (visually looks bad or disconcerting). Yes, they should be clearly distinct from the theme, and the unique shade of blue helps with that. But they shouldn't look visually bad relative to the theme's colors, especially if they look worse on certain themes versus others. Using a bold color here goes against all of the broader discussion about neutrality in the core customize UI (largely in #29158), and the more I use it, the more something doesn't feel right about the blue.

Either way, still also need a patch for the Twenty Seventeen stuff.

#14 @westonruter
5 months ago

  • Keywords has-patch added; needs-patch removed
  • Owner changed from sirbrillig to davidakennedy
  • Status changed from reopened to reviewing

@davidakennedy in 38776.2.remove-2017-rules.diff can you confirm the two rules can be removed, since you committed them?

#15 @davidakennedy
5 months ago

@westonruter Yes, those can be removed. Thanks for checking!

#16 @westonruter
5 months ago

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

In 39285:

Customize: Remove obsolete edit shortcut style rules from Twenty Seventeen.

Props celloexpressions.
Fixes #38776.

Note: See TracTickets for help on using tickets.