Skip to content

NavigationMenu: set appender color#18327

Closed
retrofox wants to merge 1 commit intotrunkfrom
try/navigation-menu-inserter-color
Closed

NavigationMenu: set appender color#18327
retrofox wants to merge 1 commit intotrunkfrom
try/navigation-menu-inserter-color

Conversation

@retrofox
Copy link
Copy Markdown
Contributor

@retrofox retrofox commented Nov 6, 2019

Description

It sets the appender color when the navigation menu has defined a text color.

How has this been tested?

Apply text color from the colors selector and confirm that the color is being applied at the appender as well.

Screenshots

before
Screen Shot 2020-02-03 at 4 37 03 PM

after
image

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.

@retrofox retrofox added [Type] Enhancement A suggestion for improvement. [Feature] List View Menu item in the top toolbar to select blocks from a list of links. labels Nov 6, 2019
@obenland
Copy link
Copy Markdown
Member

obenland commented Nov 7, 2019

By adjusting the appender color we set ourselves up for a situation where the contrast with the background is so low that it won't be visible and hampers usability.

I wonder if instead, we should give it a white or black background and border and keep it the opposite color to make it always visible. Something like this?

image

@draganescu draganescu added Needs Design Needs design efforts. Needs Design Feedback Needs general design feedback. labels Nov 7, 2019
@draganescu
Copy link
Copy Markdown
Contributor

I added the design labels as this might be an approach that impacts future blocks as well.

Copy link
Copy Markdown
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code is fine, but the solution needs to be confirmed by some design argument.

@retrofox
Copy link
Copy Markdown
Contributor Author

retrofox commented Nov 8, 2019

Testing with twenty-twenty I see how that the appender looks, pretty similar how @obenland mentioned before:

image

@karmatosed
Copy link
Copy Markdown
Member

I like the idea of a white background for it, means it's more consistent with expected appenders. My only wonder is other appenders on that page might not have it.

image

This is an example.

We do seem to colour one and not the other so there might be history there we can fall on:

image

@retrofox
Copy link
Copy Markdown
Contributor Author

retrofox commented Nov 9, 2019

I like the idea of a white background for it, means it's more consistent with expected appenders. My only wonder is other appenders on that page might not have it.

Right, consistency is a valid point. Coloring the appender with the text color (first approach on this PR) maybe make more sense?

@karmatosed
Copy link
Copy Markdown
Member

Right, consistency is a valid point. Coloring the appender with the text color (first approach on this PR) maybe make more sense?

This feels like maybe best yes.

@apeatling
Copy link
Copy Markdown
Contributor

The one issue with using text color is that it might still have low contrast if the user selects a low contrast text color. Having said that since it's a user selection I think that seems okay.

@retrofox
Copy link
Copy Markdown
Contributor Author

The one issue with using text color is that it might still have low contrast if the user selects a low contrast text color. Having said that since it's a user selection I think that seems okay.

Yes, any control should be accessible beyond the user customization. Maybe, by mistake, the user sets a wrong combination and stop seeing the appender button. Anyway, this possibility can happen in the current implementation.

@retrofox
Copy link
Copy Markdown
Contributor Author

retrofox commented Jan 16, 2020

Also, we should keep in mind the probably the styles will change soon according to changes suggested here #19681

@shaunandrews
Copy link
Copy Markdown
Contributor

I wonder if instead, we should give it a white or black background and border and keep it the opposite color to make it always visible.

I think this is the best solution. This is how the inserter works normally when using a theme with a dark background, always showing a white background behind the + icon.

@retrofox retrofox force-pushed the try/navigation-menu-inserter-color branch from 1cbc1e4 to c6a26f3 Compare February 4, 2020 00:32
@talldan talldan removed the [Feature] List View Menu item in the top toolbar to select blocks from a list of links. label Jun 11, 2020
@talldan talldan added the [Block] Navigation Affects the Navigation Block label Jun 11, 2020
@draganescu draganescu mentioned this pull request Dec 8, 2020
37 tasks
Base automatically changed from master to trunk March 1, 2021 15:42
@jasmussen
Copy link
Copy Markdown
Contributor

Since the appender now has a black background, can we close this one?

@draganescu
Copy link
Copy Markdown
Contributor

Yes.

@draganescu draganescu closed this Mar 15, 2021
@johnbillion johnbillion deleted the try/navigation-menu-inserter-color branch February 10, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants