-
Notifications
You must be signed in to change notification settings - Fork 9k
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
Allow formatting PromQL expressions in the UI #11039
Conversation
b616bcd
to
be64c47
Compare
Signed-off-by: Julius Volz <[email protected]>
be64c47
to
4d13173
Compare
Amazing! No idea about the react code. And not strong opinion on the button icon. Can it be something like |
I like the icon selected by Julius. |
Most of the time went into deciding that part, so I appreciate that 😆 |
I haven't seen such an icon around yet, but let me know if you see a better one. We could of course theoretically also create our own, but that seems like a disproportionate amount of effort. However, not sure if I like "PP". That is also not understandable unless you hover over it. |
Let's go with the current icon :) |
@juliusv would be cool to block the button when the codemirror linter returns some errors |
Signed-off-by: Julius Volz <[email protected]>
Great! I pushed one more change to the |
Signed-off-by: Julius Volz <[email protected]>
It would be, kinda. Is it worth it the extra complexity though to get that state from CodeMirror (not sure how to do it)? And from the user's perspective, if the button is blocked I would need to explain to the user why (maybe change the title property). If it is like it's now, they will at least know due to the large error display why the formatting isn't working. |
It would be great to add a green check when the query is formatted already (when we press the format button, until the input is changed again). because for now I find it confusing that it "apparently does nothing" when the query is already formatted (e.g when you click multiple times). |
Signed-off-by: Julius Volz <[email protected]>
@roidelapluie Added that (although for now I didn't make it green, maybe keeping it more low-key for such a functionality is better?): test-2022-07-20_16.16.34.mp4I am also changing the title texts now depending on the situation. |
you can do it by using the method diagnosticCount. If it is more than 0 then you the button can be blocked.
Totally yes. The message can be simply : "unable to format an incorrect expression" |
an advantage to block the button if the expression is not valid would be to not handle the http error. Based on the fact if codemirror doesn't yell at you, then your expression is correct and the server will be ok |
Signed-off-by: Julius Volz <[email protected]>
Thanks! I managed to do that now, but it's a bit laggy because the linter only responds within a few milliseconds (the linter update is not yet available in the same
IMO we should still always check those errors. There are other reasons why an API call may fail other than an invalid parameter, and who knows if the client-side checks really cover 100% of what we have in Go :) |
onExpressionChange(update.state.doc.toString()); | ||
|
||
if (exprFormatted) { | ||
setExprFormatted(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if you format the expression, then the doc changed so exprFormatted
is equal to false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also you could simply do
onExpressionChange(update.state.doc.toString());
setExprFormatted(false);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the only (weak) reason for the if
condition is to not cause yet another React state update when it's not needed (if exprFormatted
is already false
, we don't need to set it to false
), since this happens on every typed character. But yeah, probably it doesn't matter at all. I also tried to do the same with the setHasLinterErrors()
, but it led to weird feedback issues due to the lag, so now I'm just always setting it.
@juliusv I tested your branch locally and it seems there is an issue with the event. It appears that once it is formatted, the icon doesn't come back to the original state when you change the expression. Screen.Recording.2022-07-21.at.14.32.43.mov |
I also just realised that the button So I would say to be consistent: We blocked this button as well when it displayed an error, or we don't block it. By looking at the number of states of the format button, I would tend in favor of not blocking the button (even if I was proposing it initially). |
@Nexucis Ah drat, I broke that by wrapping the So I can easily fix it by just removing the |
yeah let's remove it @juliusv, it will be simpler and it's not that a huge issue that you tried to format a wrong expression at the end. It would be coherent to block these buttons, but I thought it would be simpler and it is not. So I'm more in favor in removing it now :) |
Signed-off-by: Julius Volz <[email protected]>
@roidelapluie Yup, removed it again! |
Eh sorry, meant to tag @Nexucis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks @juliusv :)
Signed-off-by: Julius Volz [email protected]
Note 1: To be 100% correct & safe I would have to also make the text editor read-only (and visually clearly so) while a format request is in-flight, but I thought that would be over-engineering it for a request that should normally finish instantaneously.
Note 2: In the video below, I introduced an artificial 1-second delay for the request so that it's possible to see the loading behavior:
test-2022-07-20_14.47.11.mp4