-
Notifications
You must be signed in to change notification settings - Fork 14k
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
fix(sql-editor): Fix run stop button color to improve usability #23892
Conversation
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.
Codecov Report
@@ Coverage Diff @@
## master #23892 +/- ##
=======================================
Coverage 68.10% 68.10%
=======================================
Files 1938 1940 +2
Lines 74999 75017 +18
Branches 8152 8155 +3
=======================================
+ Hits 51077 51093 +16
- Misses 21840 21841 +1
- Partials 2082 2083 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Left a few comments, curious to hear @kasiazjc 's thoughts
@@ -146,7 +146,9 @@ const RunQueryActionButton = ({ | |||
), | |||
trigger: 'click', | |||
} | |||
: { buttonStyle: 'primary' })} | |||
: { | |||
buttonStyle: shouldShowStopBtn ? 'danger' : 'primary', |
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.
I talked with @kasiazjc about this (she's out today), and she said we should make sure it's in line with the button in Explore. Looking at the code it appears we're using warning
instead of danger
there:
<Button onClick={onStop} buttonStyle="warning" disabled={!canStopQuery}> |
This renders the following button when Explore is fetching data:
So if we change this to warning
and also change the icon from fa fa-stop
to fa fa-stop-circle-o
then we get a very similar look and feel:
For reference, here's the button with the original icon class and danger
style:
(btw, I kinda feel like the square stop icon is more intuitive, but I'd vote for using the same icon in both places, so I'm ok either way as long as they're in line)
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.
@villebro Made the changes as requested, please take a look.
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.
Thanks @curious86 ! After chatting with @kasiazjc , we arrived at the following minor adjustment:
- Let's keep the
fa fa-stop
class in the SQL Lab button icon. - Let's swap out
fa fa-stop-circle-o
in the Explore button forfa fa-stop
After that good to go!
SUMMARY
During the running state of a query, users may find it challenging to distinguish between the stop button and the run button due to their similar color schemes. As a result, users may accidentally click the stop button, causing unintended interruption of the query's execution.
This pull request proposes to modify the color of the stop button to make it more distinct from the run button during query execution. By doing so, it will be easier for users to differentiate between the two buttons and avoid accidentally clicking the stop button, thereby preventing unintentional interruption of the query's execution.
Edit:
As part of this PR, we have also updated the explore view's stop button icon, to match it with sql lab stop button icon, for the sake of consistency.
Before:
After:
TESTING INSTRUCTIONS