Render alert fields when performing searches#221
Conversation
|
Definitely useful! |
|
Very cool! |
|
OK, this is ready for review. |
|
This would have helped reduce confusion with an issue facing a customer right now: https://sourcegraph.slack.com/archives/CJX299FGE/p1592345898255500. |
mrnugget
left a comment
There was a problem hiding this comment.
Code looks good to me! 👍
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 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.) |
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. |
|
I made two changes, and they look like this:
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. |
|
Looks good! Feel free to merge. |
|
@LawnGnome Can you add a |
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>
Yep, doing it now. |
18c5e51 to
ab0a6d4
Compare
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>
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 theactions execandsearchcommands to render any alert that comes back in a (hopefully) human readable form.Here are a couple of examples of it in use:
If we think this is useful, I'll add a couple of tests to the alert rendering and we can get this merged.