Fix nav placeholder colors and height. #31875
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
added
[Type] Bug
|
Size Change: +70 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
getdave
requested changes
May 17, 2021
There was a problem hiding this comment.
Screen.Capture.on.2021-05-17.at.09-45-31.mov
I gave this a spin. Looks like it works only if the Group has a text color set. By default when you apply a black background to the Group block it does not automatically set the text color. Therefore the text color stays as a dark color and is nearly invisible. The same thing happens to the Nav Block.
However, once you set the text color on the Group block to white, then the Nav block placeholder works.
Not sure if this is expected behaviour or not?
|
Hmmmmmm. I'll give this another spin in TT1... but if the issue is what I think it is, then yes, it's expected behavior of the nav block. The thing is, those indicators are meant to have the same color as the text. Which means if in TT1 text inside a black group is also dark/black, then so will that of the nav block indicators. I know that's not ideal, but short of the theme changing that behavior, I'm not sure this is something we can account for in the nav block. |
kjellr
approved these changes
May 17, 2021
There was a problem hiding this comment.
This seems to solve the problem for me. As @getdave noted, it won't fix things when there's no text color set (or when the text color is unreadable in general), but it does work for when there is a text color set. And that wasn't the case before this PR:
Group block (white text, black background) before:
Group block (white text, black background) after:
That should fix the issues I was seeing in #31610.
|
Thank you Kjell. @getdave mind if I land this? |
getdave
approved these changes
May 17, 2021
|
As always, I'm easy to find if/when follow-ups need to be made. |
|
(also, thanks) |
jorgefilipecosta
pushed a commit
that referenced
this pull request
May 18, 2021
Co-authored-by: André <[email protected]> (+3 squashed commits) Squashed commits: [994e5e3] Add link color as an element mechanism. [e4a72bc] Remove existing link color [7fcd57c] Fix nav placeholder colors and height. (#31875)
oandregal
pushed a commit
that referenced
this pull request
May 19, 2021
Co-authored-by: André <[email protected]> (+3 squashed commits) Squashed commits: [994e5e3] Add link color as an element mechanism. [e4a72bc] Remove existing link color [7fcd57c] Fix nav placeholder colors and height. (#31875)
jorgefilipecosta
added a commit
that referenced
this pull request
May 19, 2021
Co-authored-by: André <[email protected]> (+3 squashed commits) Squashed commits: [994e5e3] Add link color as an element mechanism. [e4a72bc] Remove existing link color [7fcd57c] Fix nav placeholder colors and height. (#31875)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #31841.
This PR fixes a regression with the placeholder state of the navigation block, where colors weren't inherited.
It also fixes a small edgecase where the setup state would jump.
How has this been tested?
Before:
After:
Checklist:
*.native.jsfiles for terms that need renaming or removal).