Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

command/format: Render null in dark gray #19616

Merged
merged 1 commit into from
Jan 11, 2019
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Dec 11, 2018

I thought it would be even nicer to render the whole line as red, but then the whole -> null part becomes kinda redundant and I assume we want to keep it there for non-colorized outputs as minus signs alone might not be immediately obvious.

Before

screen shot 2018-12-11 at 20 58 02

After

screen shot 2019-01-11 at 13 51 54

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Seems reasonable!

I like it not all being red, since I want to avoid any actual information being colorized to ensure that it'll remain readable on terminals with weirdly-configured color settings. Since the -> null part is more of an "icon" than actual information it's not so much of a big deal if it's hard to actually read the text.

@apparentlymart
Copy link
Contributor

Oh, one thing we should check here:

If a resource is being updated to set something that was previously set to null, and that thing requires replacement, does the # requires replacement annotation still stand out obviously or does it get visually drowned out by the red -> null noise?

That annotation being rendered in bold might be enough to avoid that, but if not then I think it's better to leave it uncolored so that we can reserve red text for more significant situations.

@apparentlymart
Copy link
Contributor

Looking at this again with fresh eyes, I think that this makes the nulls a little too prominent in the output here, which risks distracting from other information we want to keep prominent, like whether something is going to be replaced.

Since the -> null annotations are already redundant with the - markers at the start, I wonder if instead we should make them [dark_gray] to de-emphasize them. I added these extra -> null markers in there because I got initial feedback that the minus signs alone were confusing, but they also add a lot of visual noise to the diff. Hopefully de-emphasizing them will make it easier to visually skim and see the old values, which are the main interesting thing to see so you can understand exactly what is being deleted.

(I'd include the -> in the de-emphasizing too.)

@radeksimko radeksimko changed the title command/format: Render null in red command/format: Render null in dark grey Dec 22, 2018
@radeksimko
Copy link
Member Author

radeksimko commented Jan 11, 2019

@apparentlymart Thanks for the feedback. Dark gray does look better in this context. See updated screenshot above.

Are you happy for me to merge this?

@radeksimko radeksimko changed the title command/format: Render null in dark grey command/format: Render null in dark gray Jan 11, 2019
@apparentlymart apparentlymart added this to the v0.12.0 milestone Jan 11, 2019
@radeksimko radeksimko merged commit bc4b7ca into master Jan 11, 2019
@radeksimko radeksimko deleted the f-cmd-fmt-red-null branch January 11, 2019 19:27
@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants