You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/04/06 11:11:55 UTC
[GitHub] [superset] kgabryje opened a new pull request, #19558: feat(explore): Redesign of Run/Save buttons
kgabryje opened a new pull request, #19558:
URL: https://github.com/apache/superset/pull/19558
### SUMMARY
This PR moves the "Save" button to chart header and "Run" button (renamed to "Create/Update chart") to the bottom of control panel.
Full list of changes:
- "Save" button moved to the right side if chart header
- Renamed "Run" button to "Create chart" if it's a new chart (no query has been made yet) or "Update chart" if queries were run before on that chart
- Moved "Create/Update chart" button to the bottom of control panel - it's sticky, meaning it always stays in place when user scrolls the control panel
- "Data/Customize" tab bar is sticky - only the content of tab gets scrolled, the tab bar stays in place
- Error icon moved from next to "Run/Save" buttons to "Data" tab header
- Change empty state message when creating chart and all controls have been added - "Your chart is ready to go!"
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://user-images.githubusercontent.com/15073128/161961889-643b4ec0-97fb-4a9d-9b6c-56a0174f6498.mov
### TESTING INSTRUCTIONS
1. Create new chart
2. Verify that the button at the bottom of control panel says "Create chart" and is disabled
3. Add necessary controls
4. "Create chart" button should not be disabled now + the message in empty state changed to a prompt to click the button
5. Click "Create chart" button
6. Verify that query was run and the name of the button changed to "Update chart"
7. Verify that "Save" button in the chart header works as before
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
@kasiazjc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
geido commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1095370381
@kgabryje not sure if this is intended but when you switch viz types, the button stays at "UPDATE CHART". This happens when some metrics are kepts and also when all metrics are dropped (basically starting from a blank state). I think it should say "CREATE CHART" to be consistent, especially on the latter case where no metrics are kept.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
geido commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1095102549
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1093021777
> When a control that's not a "render trigger" changes, user sees an alert banner. That lets them know that they should click update chart button, but at the same time they can still see their chart.
That's better than the overlay! I like that the user can see the chart. I would just add the update button right there instead of floating on the left panel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1092626293
Three observations:
1) Currently the Run button only becomes "enabled" (well, it's not really ever disabled, the background just becomes white) when the query result is stale from controls that affect the queries (see before video). In the new version, the "Update chart" button is "enabled" all the time (see after video).
2) The "Run Query" in the chart container should probably be changed to "Update chart" to be in line with the other button.
3) When there are errors, currently the Save button is disabled. In the new version, the Save button appears to be enabled all the time.
A more general observation: I believe one of the reasons the button is currently called "Run" is to reflect that it executes a query (changing controls that only affect the visuals of the chart don't require rerunning the chart, as they're applied on the fly). Renaming it to "Create chart" and "Update chart" *may* cause confusing due to the following:
- Users will think they have to press "Update chart" when they make changes that don't affect the query
- When the button is called "Create chart", there's risk that users will think it will also save the chart.
However, my comments should be taken with a grain of salt, as I'm probably too used to the old behavior.
Other than that I love the new design!
Before:
https://user-images.githubusercontent.com/33317356/162401526-5282bf89-9d89-4022-8c84-e96119de0628.mp4
After:
https://user-images.githubusercontent.com/33317356/162401608-33ebbb17-6ccc-47ea-80c7-2e633ae9bf0e.mp4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846426045
##########
superset-frontend/src/explore/components/RunQueryButton/index.tsx:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { ReactNode } from 'react';
+import { t } from '@superset-ui/core';
+import Button from 'src/components/Button';
+
+export type RunQueryButtonProps = {
+ loading: boolean;
+ onQuery: () => void;
+ onStop: () => void;
+ errorMessage: ReactNode;
+ isNewChart: boolean;
+ canStopQuery: boolean;
+ chartIsStale: boolean;
+};
+
+export const RunQueryButton = ({
+ loading,
+ onQuery,
+ onStop,
+ errorMessage,
+ isNewChart,
+ canStopQuery,
+ chartIsStale,
+}: RunQueryButtonProps) =>
+ loading ? (
+ <Button onClick={onStop} buttonStyle="warning" disabled={!canStopQuery}>
+ <i className="fa fa-stop-circle-o" /> {t('Stop')}
Review Comment:
Can we use an icon from the `Icons` component?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1090308546
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1092774793
@villebro Fixes implemented.
1. Update button is now secondary instead of primary if chart is not stale
<img width="1792" alt="image" src="https://user-images.githubusercontent.com/15073128/162428944-d9fe0bff-8523-4d3a-a240-e5ef6068ec06.png">
2. Save button is disabled when there are errors + tooltip
<img width="301" alt="image" src="https://user-images.githubusercontent.com/15073128/162429079-ae01c9c6-4f72-4b83-8e3e-47b4c9f5d8cf.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1093019512
@michael-s-molina Thanks for comments! Sneak preview for the replacement of overlay with "run query" button:
<img width="1792" alt="image" src="https://user-images.githubusercontent.com/15073128/162476149-78ae2c7d-a3c1-42cb-9497-3a39da667057.png">
When a control that's not a "render trigger" changes, user sees an alert banner. That lets them know that they should click update chart button, but at the same time they can still see their chart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1092634093
> 1. Currently the Run button only becomes "enabled" (well, it's not really ever disabled, the background just becomes white) when the query result is stale from controls that affect the queries (see before video). In the new version, the "Update chart" button is "enabled" all the time (see after video).
Good point, will change!
> 2. The "Run Query" in the chart container should probably be changed to "Update chart" to be in line with the other button.
Today I'm going to open a PR that totally removes the overlay with "Run query". It will be replaced by an alert banner that tells the user that something has changed in the controls. Thanks to that, the user will still be able to see the chart while making changes in the controls.
> 3. When there are errors, currently the Save button is disabled. In the new version, the Save button appears to be enabled all the time.
Oooooops...
> A more general observation: I believe one of the reasons the button is currently called "Run" is to reflect that it executes a query (changing controls that only affect the visuals of the chart don't require rerunning the chart, as they're applied on the fly). Renaming it to "Create chart" and "Update chart" _may_ cause confusing due to the following:
>
> * Users will think they have to press "Update chart" when they make changes that don't affect the query
> * When the button is called "Create chart", there's risk that users will think it will also save the chart.
I'll let @kasiazjc address that one 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r847195627
##########
superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx:
##########
@@ -86,8 +86,8 @@ const MenuItemWithCheckboxContainer = styled.div`
const MenuTrigger = styled(Button)`
${({ theme }) => css`
Review Comment:
It's not a blocker for me, it's just unnecessary because of `styled`. I don't know why the IDE is not highlighting the CSS inside the `styled` tag. If I had to choose I would remove it just because someone reading the code without the same IDE or plugin installed wouldn't understand why there's a `css` clause inside `styled`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1090253763
# [Codecov](https://codecov.io/gh/apache/superset/pull/19558?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19558](https://codecov.io/gh/apache/superset/pull/19558?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f7cf2cc) into [master](https://codecov.io/gh/apache/superset/commit/2de5e6fac49d711d45b4d7e0c6e4a77e5711c8f1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2de5e6f) will **decrease** coverage by `0.00%`.
> The diff coverage is `43.33%`.
```diff
@@ Coverage Diff @@
## master #19558 +/- ##
==========================================
- Coverage 66.66% 66.65% -0.01%
==========================================
Files 1678 1680 +2
Lines 64261 64271 +10
Branches 6561 6562 +1
==========================================
+ Hits 42837 42842 +5
- Misses 19723 19731 +8
+ Partials 1701 1698 -3
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.46% <43.33%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../legacy-plugin-chart-country-map/src/CountryMap.js](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNvdW50cnktbWFwL3NyYy9Db3VudHJ5TWFwLmpz) | `0.00% <ø> (ø)` | |
| [...y-plugin-chart-country-map/src/ReactCountryMap.jsx](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNvdW50cnktbWFwL3NyYy9SZWFjdENvdW50cnlNYXAuanN4) | `0.00% <0.00%> (ø)` | |
| [...y-plugin-chart-sankey-loop/src/ReactSankeyLoop.jsx](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LXNhbmtleS1sb29wL3NyYy9SZWFjdFNhbmtleUxvb3AuanN4) | `0.00% <0.00%> (ø)` | |
| [.../legacy-plugin-chart-sankey-loop/src/SankeyLoop.js](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LXNhbmtleS1sb29wL3NyYy9TYW5rZXlMb29wLmpz) | `0.00% <ø> (ø)` | |
| [...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlclZpei50c3g=) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `60.39% <0.00%> (-0.35%)` | :arrow_down: |
| [superset-frontend/src/components/Chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnQuanN4) | `50.00% <0.00%> (-2.46%)` | :arrow_down: |
| [.../components/ExploreAdditionalActionsMenu/index.jsx](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQWRkaXRpb25hbEFjdGlvbnNNZW51L2luZGV4LmpzeA==) | `57.14% <ø> (ø)` | |
| [superset/models/sql\_lab.py](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL3NxbF9sYWIucHk=) | `91.55% <ø> (ø)` | |
| [.../src/explore/components/ControlPanelsContainer.tsx](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLnRzeA==) | `80.18% <75.00%> (+0.39%)` | :arrow_up: |
| ... and [5 more](https://codecov.io/gh/apache/superset/pull/19558/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19558?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19558?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c4baa82...f7cf2cc](https://codecov.io/gh/apache/superset/pull/19558?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1093009369
> Users will think they have to press "Update chart" when they make changes that don't affect the query
I agree with @villebro on this one
I really like the new design, things are getting less cluttered, and the Explore screen seems simpler while keeping the same features. I do have a different preference for the organization for the create/update buttons though. I personally don't like things floating on top of other things, especially on top of the configuration panel where we have many controls. It gives me a crowded feeling.
One idea would be to move the create button directly below the empty message, it seems very intuitive because I'm reading that my chart is ready to go and there would be a button right there to create it.
About the update, I liked the previous version that showed an overlay on top of the chart with a button to update. I like this approach because:
- It shows the user (with the overlay + button) that they did a configuration that needs an explicit update request
- The button to update is right there on the overlay. We could even add a message indicating that the user needs to update to see the changes reflected on the chart
- When a user does a configuration on the left panel that doesn't require an explicit update request, the chart simply updates and there's no button on the screen that could lead to confusion.
For the case where the user wants to re-execute the query without having made any modification to the configurations, to bust the cache, for example, we could have a "Refresh data" option in the secondary actions popover.
These are just ideas, I don't consider them as blockers for the PR in case you choose a different direction 😉
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1098217923
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r847186196
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -431,6 +466,32 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
[handleClearFormClick, handleContinueClick, hasControlsTransferred],
);
+ const dataTabTitle = useMemo(
+ () => (
+ <>
+ <span>{t('Data')}</span>
+ {props.errorMessage && (
+ <span
+ css={(theme: SupersetTheme) => css`
+ font-size: ${theme.typography.sizes.xs}px;
+ margin-left: ${theme.gridUnit * 2}px;
+ `}
+ >
+ {' '}
+ <Tooltip
+ id="query-error-tooltip"
+ placement="right"
+ title={props.errorMessage}
+ >
+ <i className="fa fa-exclamation-circle text-danger fa-lg" />
Review Comment:
That's the same icon as the one that's displayed next to controls. I agree that we should drop font-awesome icons in favor of antd, but changing that here would also require changing the danger icon in other places in control panel. Let's do that in a separate PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r847187522
##########
superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx:
##########
@@ -86,8 +86,8 @@ const MenuItemWithCheckboxContainer = styled.div`
const MenuTrigger = styled(Button)`
${({ theme }) => css`
Review Comment:
Is there any reason not to use that? With `css`, my IDE is able to apply css highlighting and suggest css syntax. Without it, it treats the content like regular text
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1093066096
@villebro Ephemeral environment spinning up at http://35.160.60.86:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846362438
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -76,13 +85,32 @@ export type ExpandedControlPanelSectionConfig = Omit<
controlSetRows: ExpandedControlItem[][];
};
+const ActionButtonsContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ position: sticky;
+ bottom: 0;
+ flex-direction: column;
+ align-items: center;
+ padding: ${theme.gridUnit * 4}px;
+ background: linear-gradient(transparent, white 25%);
+
+ & > button {
+ min-width: ${theme.gridUnit * 39}px;
Review Comment:
Is this really related to grid units or we can use a defined width?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846361362
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -76,13 +85,32 @@ export type ExpandedControlPanelSectionConfig = Omit<
controlSetRows: ExpandedControlItem[][];
};
+const ActionButtonsContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ position: sticky;
+ bottom: 0;
+ flex-direction: column;
+ align-items: center;
+ padding: ${theme.gridUnit * 4}px;
+ background: linear-gradient(transparent, white 25%);
Review Comment:
```suggestion
background: linear-gradient(transparent, ${theme.colors.grayscale.light5} 25%);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846372171
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -288,6 +288,23 @@ class Chart extends React.PureComponent {
);
}
+ if (
+ !isLoading &&
+ !chartAlert &&
+ isFaded &&
+ ensureIsArray(queriesResponse).length === 0
+ ) {
+ return (
+ <EmptyStateBig
+ title={t('Your chart is ready to go!')}
+ description={t(
+ 'Click on "Create chart" button in the control panel on the left to preview a visualisation',
Review Comment:
In the US, yes. We're weird. We also optimize and scrutinize and so forth. Maybe one day we'll have American English and English localizations fleshed out :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1095256367
@geido Ephemeral environment spinning up at http://18.237.253.23:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846360193
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +60,8 @@ import { ChartState } from 'src/explore/types';
import ControlRow from './ControlRow';
import Control from './Control';
import { ControlPanelAlert } from './ControlPanelAlert';
+import { RunQueryButton } from './RunQueryButton';
+import { Tooltip } from '../../components/Tooltip';
Review Comment:
Don't forget to change the order of the imports 😉
```suggestion
import { Tooltip } from 'src/components/Tooltip';
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846421938
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -111,6 +114,13 @@ const StyledButtons = styled.span`
`}
`;
+const SaveButtonContainer = styled.div`
+ ${({ theme }) =>
+ css`
Review Comment:
```suggestion
`
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r847189504
##########
superset-frontend/src/explore/components/RunQueryButton/index.tsx:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { ReactNode } from 'react';
+import { t } from '@superset-ui/core';
+import Button from 'src/components/Button';
+
+export type RunQueryButtonProps = {
+ loading: boolean;
+ onQuery: () => void;
+ onStop: () => void;
+ errorMessage: ReactNode;
+ isNewChart: boolean;
+ canStopQuery: boolean;
+ chartIsStale: boolean;
+};
+
+export const RunQueryButton = ({
+ loading,
+ onQuery,
+ onStop,
+ errorMessage,
+ isNewChart,
+ canStopQuery,
+ chartIsStale,
+}: RunQueryButtonProps) =>
+ loading ? (
+ <Button onClick={onStop} buttonStyle="warning" disabled={!canStopQuery}>
+ <i className="fa fa-stop-circle-o" /> {t('Stop')}
Review Comment:
I'm not sure if we have an icon that looks like that one. Let's make a separate PR "Replace fa icons in antd in control panel" that will handle this icon and those warning icons mentioned above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1090319682
@kgabryje Ephemeral environment spinning up at http://54.189.133.181:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kasiazjc commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1092649292
> > 1. Currently the Run button only becomes "enabled" (well, it's not really ever disabled, the background just becomes white) when the query result is stale from controls that affect the queries (see before video). In the new version, the "Update chart" button is "enabled" all the time (see after video).
>
>
>
> Good point, will change!
>
>
>
> > 2. The "Run Query" in the chart container should probably be changed to "Update chart" to be in line with the other button.
>
>
>
> Today I'm going to open a PR that totally removes the overlay with "Run query". It will be replaced by an alert banner that tells the user that something has changed in the controls. Thanks to that, the user will still be able to see the chart while making changes in the controls.
>
>
>
> > 3. When there are errors, currently the Save button is disabled. In the new version, the Save button appears to be enabled all the time.
>
>
>
> Oooooops...
>
>
>
>
>
> > A more general observation: I believe one of the reasons the button is currently called "Run" is to reflect that it executes a query (changing controls that only affect the visuals of the chart don't require rerunning the chart, as they're applied on the fly). Renaming it to "Create chart" and "Update chart" _may_ cause confusing due to the following:
>
> >
>
> > * Users will think they have to press "Update chart" when they make changes that don't affect the query
>
> > * When the button is called "Create chart", there's risk that users will think it will also save the chart.
>
>
>
> I'll let @kasiazjc address that one 🙂
>
>
>
>
@villebro thanks for ten Feedback 🙏🏻
For the copy we got feedback via Aha/GitHub, that "run query" is confusing as some people do not associate creating chart with running the query.
Hopefully "save" button in the header should solve the second problem.
I was also thinking about naming the button "preview chart", but then it completely removes the indication that the query was ran, as preview seems less... invasive if that makes sense.
Let's stick with this copy for now and we'll iterate if needed :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r847422405
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -231,11 +231,13 @@ export class ExploreChartHeader extends React.PureComponent {
isStarred,
sliceUpdated,
sliceName,
+ onSaveChart,
+ saveDisabled,
} = this.props;
const { latestQueryFormData, sliceFormData } = chart;
const oldSliceName = slice?.slice_name;
return (
- <StyledHeader id="slice-header">
+ <div id="slice-header" css={headerStyles}>
Review Comment:
Same comment as the other one for the usage of `css`
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -475,6 +538,17 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
</Tabs.TabPane>
)}
</ControlPanelsTabs>
+ <div css={actionButtonsContainerStyles}>
Review Comment:
A super nit but I think this conflicts a little bit with the Emotion guidelines that we have. The goal of using `css` is to have it closer to the component where it's applied, especially if it's for a single use. In this case, you are using `css` but moving it far from the component to avoid taking too much space here I guess, so probably we should go with the standard `styled.div` instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846359118
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -288,6 +288,23 @@ class Chart extends React.PureComponent {
);
}
+ if (
+ !isLoading &&
+ !chartAlert &&
+ isFaded &&
+ ensureIsArray(queriesResponse).length === 0
+ ) {
+ return (
+ <EmptyStateBig
+ title={t('Your chart is ready to go!')}
+ description={t(
+ 'Click on "Create chart" button in the control panel on the left to preview a visualisation',
Review Comment:
@rusackas Help me out here but I think "visualiZation" is more generally accepted in the US right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846424586
##########
superset-frontend/src/explore/components/RunQueryButton/RunQueryButton.test.tsx:
##########
@@ -0,0 +1,78 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import userEvent from '@testing-library/user-event';
+import { render, screen } from 'spec/helpers/testing-library';
+import { RunQueryButton } from './index';
+
+const createProps = (overrides: Record<string, any> = {}) => ({
+ loading: false,
+ onQuery: jest.fn(),
+ onStop: jest.fn(),
+ errorMessage: null,
+ isNewChart: false,
+ canStopQuery: true,
+ chartIsStale: true,
+ ...overrides,
+});
+
+describe('RunQueryButton', () => {
Review Comment:
[Avoid nesting when you're testing](https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#avoid-nesting-when-youre-testing)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846420499
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -431,6 +466,32 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
[handleClearFormClick, handleContinueClick, hasControlsTransferred],
);
+ const dataTabTitle = useMemo(
+ () => (
+ <>
+ <span>{t('Data')}</span>
+ {props.errorMessage && (
+ <span
+ css={(theme: SupersetTheme) => css`
+ font-size: ${theme.typography.sizes.xs}px;
+ margin-left: ${theme.gridUnit * 2}px;
+ `}
+ >
+ {' '}
+ <Tooltip
+ id="query-error-tooltip"
+ placement="right"
+ title={props.errorMessage}
+ >
+ <i className="fa fa-exclamation-circle text-danger fa-lg" />
Review Comment:
Shouldn't we use an icon from the `Icons` component?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846421638
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -55,8 +58,18 @@ const propTypes = {
ownState: PropTypes.object,
timeout: PropTypes.number,
chart: chartPropShape,
+ saveDisabled: PropTypes.bool,
};
+const SaveButton = styled(Button)`
+ ${({ theme }) => css`
Review Comment:
```suggestion
${({ theme }) => `
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1093062345
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kasiazjc commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1095061959
> LGTM. Thanks for the awesome changes @kgabryje @kasiazjc!
And thanks for feedback and discussion @michael-s-molina 💞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
geido commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1098156865
> > @kgabryje not sure if this is intended but when you switch viz types, the button stays at "UPDATE CHART". This happens when some metrics are kept and also when all metrics are dropped (basically starting from a blank state). I think it should say "CREATE CHART" to be consistent, especially in the latter case where no metrics are kept.
>
> That's a really good question. It was intentional, yes, because from my point of view if a chart exists (i.e. it's been rendered), then changing the viz type is updating, not creating from scratch, even if controls were reset. After all, that chart is going to have the same reference at the list of charts and is going to take the old chart's place in any dashboard where it was added. So I think in that case we're updating a chart, not creating something new.
> What do you think?
Makes sense. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje merged pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje merged PR #19558:
URL: https://github.com/apache/superset/pull/19558
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19558:
URL: https://github.com/apache/superset/pull/19558#issuecomment-1097935852
> @kgabryje not sure if this is intended but when you switch viz types, the button stays at "UPDATE CHART". This happens when some metrics are kept and also when all metrics are dropped (basically starting from a blank state). I think it should say "CREATE CHART" to be consistent, especially in the latter case where no metrics are kept.
That's a really good question. It was intentional, yes, because from my point of view if a chart exists (i.e. it's been rendered), then changing the viz type is updating, not creating from scratch, even if controls were reset. After all, that chart is going to have the same reference at the list of charts and is going to take the old chart's place in any dashboard where it was added. So I think in that case we're updating a chart, not creating something new.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846373333
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -76,13 +85,32 @@ export type ExpandedControlPanelSectionConfig = Omit<
controlSetRows: ExpandedControlItem[][];
};
+const ActionButtonsContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ position: sticky;
+ bottom: 0;
+ flex-direction: column;
+ align-items: center;
+ padding: ${theme.gridUnit * 4}px;
+ background: linear-gradient(transparent, white 25%);
Review Comment:
@michael-s-molina @geido this "25%" reminds me... I kind of think we should have a series of standard opacity stops and gradient stops in our theme.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846361362
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -76,13 +85,32 @@ export type ExpandedControlPanelSectionConfig = Omit<
controlSetRows: ExpandedControlItem[][];
};
+const ActionButtonsContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ position: sticky;
+ bottom: 0;
+ flex-direction: column;
+ align-items: center;
+ padding: ${theme.gridUnit * 4}px;
+ background: linear-gradient(transparent, white 25%);
Review Comment:
```suggestion
background: linear-gradient(transparent, ${theme.colors.grayscale.light5} ${theme.opacity.mediumLight});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846421210
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -76,13 +85,32 @@ export type ExpandedControlPanelSectionConfig = Omit<
controlSetRows: ExpandedControlItem[][];
};
+const ActionButtonsContainer = styled.div`
+ ${({ theme }) => css`
Review Comment:
```suggestion
${({ theme }) => `
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #19558: feat(explore): Redesign of Run/Save buttons
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19558:
URL: https://github.com/apache/superset/pull/19558#discussion_r846420958
##########
superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx:
##########
@@ -86,8 +86,8 @@ const MenuItemWithCheckboxContainer = styled.div`
const MenuTrigger = styled(Button)`
${({ theme }) => css`
Review Comment:
```suggestion
${({ theme }) => `
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org