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

Render alert fields when performing searches #221

Merged
merged 10 commits into from
Jun 22, 2020

Conversation

LawnGnome
Copy link
Contributor

We have all this useful information in the web UI that comes back from GraphQL, but isn't exposed in src. This PR wires up a common renderer to the actions exec and search commands to render any alert that comes back in a (hopefully) human readable form.

Here are a couple of examples of it in use:

image
image

If we think this is useful, I'll add a couple of tests to the alert rendering and we can get this merged.

@LawnGnome LawnGnome added enhancement New feature or request action-exec Everything related to the action exec functionality labels Jun 9, 2020
@sqs
Copy link
Member

sqs commented Jun 9, 2020

Definitely useful!

@eseliger
Copy link
Member

eseliger commented Jun 9, 2020

Very cool!

@LawnGnome LawnGnome marked this pull request as ready for review June 9, 2020 19:10
@LawnGnome
Copy link
Contributor Author

OK, this is ready for review.

@LawnGnome LawnGnome requested a review from mrnugget June 16, 2020 16:20
@sqs
Copy link
Member

sqs commented Jun 16, 2020

This would have helped reduce confusion with an issue facing a customer right now: https://sourcegraph.slack.com/archives/CJX299FGE/p1592345898255500.

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Code looks good to me! 👍

Visually, though, I think this might be a bit hard to parse, since we have up to 4 colors:

screenshot_2020-06-17_11 08 59@2x

@LawnGnome
Copy link
Contributor Author

Visually, though, I think this might be a bit hard to parse, since we have up to 4 colors:

Yeah, I'm not going to say the colours were picked totally at random, but it's not far off. Here was my logic:

  • The actual alert gets shown in red, the colour of errors.
  • The description that goes along with it to describe a possible course of action is orange, to look like a lesser part of the error.
  • The suggestions are in blue to highlight that you might want to copy/paste them.
  • Everything else is in the default colour.

The warning is rendered separately in yellow.

I'm open to suggestions: unifying the description colour with the warning might make sense, for example, and I'm not tied to blue for the suggestions. (We could also bold them instead of changing colour.)

@mrnugget
Copy link
Contributor

I'm open to suggestions: unifying the description colour with the warning might make sense, for example, and I'm not tied to blue for the suggestions. (We could also bold them instead of changing colour.)

I think having the description in red also makes sense, since you could then visually group the messages: yellow warning because it didn't find anything, red error shows why it didn't find anything, then suggestions.

But don't feel blocked by this comment. I think the code can be merged. If you do have an idea to improve the colors though, go for it. If not, we can fix it later.

@LawnGnome
Copy link
Contributor Author

I made two changes, and they look like this:

image

I adopted @mrnugget's suggestion of unifying the alert and description colours, and I think it looks good, so let's go with that. I also added an indent on the lines past the alert — as long as the emoji is rendered as a double-width character (which I think is true on most platforms), the alert will now appear together.

@mrnugget
Copy link
Contributor

Looks good! Feel free to merge.

@mrnugget
Copy link
Contributor

@LawnGnome Can you add a CHANGELOG.md entry?

cmd/src/search_alert_test.go Outdated Show resolved Hide resolved
@LawnGnome
Copy link
Contributor Author

@LawnGnome Can you add a CHANGELOG.md entry?

Yep, doing it now.

@LawnGnome LawnGnome force-pushed the aharvey/expose-search-result-alert branch from 18c5e51 to ab0a6d4 Compare June 22, 2020 16:43
@LawnGnome LawnGnome merged commit 1f88596 into master Jun 22, 2020
scjohns pushed a commit that referenced this pull request Apr 24, 2023
This wires up a common renderer to the `actions exec` and `search` commands to
render any alert that comes back in a (hopefully) human readable form.

Co-authored-by: Erik Seliger <erikseliger@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action-exec Everything related to the action exec functionality enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants