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.

Sign up for GitHub

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

Jump to bottom

fix(sql-editor): Fix run stop button color to improve usability #23892

Merged
merged 3 commits into from
May 3, 2023
Merged

fix(sql-editor): Fix run stop button color to improve usability #23892

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
911403b
Update the color of the run button when it is in a running state
May 1, 2023
4256550
stop button styles updated to match explore button scheme
May 2, 2023
93062b2
sql lab and explore view run query button updated
May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion superset-frontend/src/SqlLab/components/RunQueryActionButton/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,9 @@ const RunQueryActionButton = ({
),
trigger: 'click',
}
: { buttonStyle: 'primary' })}
: {
buttonStyle: shouldShowStopBtn ? 'danger' : 'primary',
Copy link
Member

@villebro villebro May 2, 2023 •
edited
Loading

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:

superset/superset-frontend/src/explore/components/RunQueryButton/index.tsx

Line 44 in 3c381c5

<Button onClick={onStop} buttonStyle="warning" disabled={!canStopQuery}>

This renders the following button when Explore is fetching data:

image

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:

image

For reference, here's the button with the original icon class and danger style:
image

(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)

Copy link
Contributor Author

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.

Copy link
Member

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 for fa fa-stop

After that good to go!

})}
>
{buildText(shouldShowStopBtn, selectedText)}
</ButtonComponent>
Expand Down