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/13 17:48:48 UTC
[GitHub] [superset] kgabryje opened a new pull request, #19696: feat(explore): Replace overlay with alert banner when chart controls change
kgabryje opened a new pull request, #19696:
URL: https://github.com/apache/superset/pull/19696
### SUMMARY
Before, when user made some changes to controls, we displayed an overlay with "Run query" button. This PR removes that overlay and introduces an alert banner that tells the user that some controls have changed and that they should click "Update chart" button. Thanks to that, the user can still view their chart while doing changes to controls.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Uploading Screen Recording 2022-04-13 at 18.50.32.mov…
### TESTING INSTRUCTIONS
1. Create a chart
2. Do some changes in controls and verify that an alert banner showed up.
3. After clicking "Update chart" the banner should disappear.
4. When banner is visible, the chart should still respond to events that trigger rerendering, such as changing the width of browser window.
### 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
CC @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] kgabryje commented on a diff in pull request #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #19696:
URL: https://github.com/apache/superset/pull/19696#discussion_r849748716
##########
superset-frontend/src/explore/components/ExploreAlert.tsx:
##########
@@ -0,0 +1,127 @@
+/**
Review Comment:
This is `ControlPanelAlert`, just renamed. No need to review this file
--
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 #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
kgabryje merged PR #19696:
URL: https://github.com/apache/superset/pull/19696
--
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 #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19696:
URL: https://github.com/apache/superset/pull/19696#issuecomment-1099106128
@kgabryje Ephemeral environment spinning up at http://54.200.67.72: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] villebro commented on a diff in pull request #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #19696:
URL: https://github.com/apache/superset/pull/19696#discussion_r852643855
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -255,34 +252,24 @@ class Chart extends React.PureComponent {
chartAlert,
chartStatus,
errorMessage,
- onQuery,
- refreshOverlayVisible,
+ chartIsStale,
queriesResponse = [],
isDeactivatedViz = false,
width,
} = this.props;
const isLoading = chartStatus === 'loading';
- const isFaded = refreshOverlayVisible && !errorMessage;
+ const isFaded = chartIsStale && !errorMessage;
Review Comment:
As we're no longer fading, this variable should probably be renamed
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -108,25 +107,23 @@ const Styles = styled.div`
}
`;
-const RefreshOverlayWrapper = styled.div`
- position: absolute;
- top: 0;
- left: 0;
- width: 100%;
- height: 100%;
- display: flex;
- align-items: center;
- justify-content: center;
-`;
-
const MonospaceDiv = styled.div`
font-family: ${({ theme }) => theme.typography.families.monospace};
- white-space: pre;
word-break: break-word;
overflow-x: auto;
white-space: pre-wrap;
`;
+const requiredFieldsMissingWarning = isFeatureEnabled(
+ FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP,
+)
+ ? t(
+ 'Drag and drop values into highlighted field(s) in the control panel. Then run the query by clicking on the "Create chart" button.',
+ )
+ : t(
+ 'Select values in highlighted field(s) in the control panel. Then run the query by clicking on the "Create chart" button.',
+ );
Review Comment:
Not something you're changing here, but to get rid of a ternary, I wonder if we couldn't get away with just one message here. As the current default supports both dragging and dropping *and* clicking on the ghost button, the "Select values in highlighted field(s) in the control panel" message could IMO be an ok message for both DnD and non-DnD scenarios.
##########
superset-frontend/src/explore/components/ExploreChartPanel.jsx:
##########
@@ -109,6 +126,16 @@ const Styles = styled.div`
}
`;
+const requiredFieldsMissingWarning = isFeatureEnabled(
+ FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP,
+)
+ ? t(
+ 'Drag and drop values into highlighted field(s) in the control panel. Then run the query by clicking on the "Update chart" button.',
+ )
+ : t(
+ 'Select values in highlighted field(s) in the control panel. Then run the query by clicking on the "Update chart" button.',
+ );
Review Comment:
As this is identical to the text in `src/components/Chart/Chart.tsx`, we should perhaps consider placing these strings in a `constants.ts` to ensure they stay the same (otherwise someone might only update one but not the other).
--
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] jinghua-qa commented on pull request #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #19696:
URL: https://github.com/apache/superset/pull/19696#issuecomment-1100575854
tested PR locally, did not find major issue, LGTM
--
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 #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19696:
URL: https://github.com/apache/superset/pull/19696#issuecomment-1102620795
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 #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #19696:
URL: https://github.com/apache/superset/pull/19696#discussion_r852724895
##########
superset-frontend/src/explore/components/ExploreChartPanel.jsx:
##########
@@ -109,6 +126,16 @@ const Styles = styled.div`
}
`;
+const requiredFieldsMissingWarning = isFeatureEnabled(
+ FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP,
+)
+ ? t(
+ 'Drag and drop values into highlighted field(s) in the control panel. Then run the query by clicking on the "Update chart" button.',
+ )
+ : t(
+ 'Select values in highlighted field(s) in the control panel. Then run the query by clicking on the "Update chart" button.',
+ );
Review Comment:
Not exactly identical - the other one says "Create chart", this one "Update chart". Though we should probably make it more generic, like move to a function with argument `isCreating` or `isUpdating`
--
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 #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19696:
URL: https://github.com/apache/superset/pull/19696#issuecomment-1098365276
# [Codecov](https://codecov.io/gh/apache/superset/pull/19696?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 [#19696](https://codecov.io/gh/apache/superset/pull/19696?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0397a6a) into [master](https://codecov.io/gh/apache/superset/commit/c8304a2821cc86d01e3e3c01ee597c94b1fb64e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8304a2) will **increase** coverage by `0.01%`.
> The diff coverage is `68.25%`.
> :exclamation: Current head 0397a6a differs from pull request most recent head d6a2e0b. Consider uploading reports for the commit d6a2e0b to get more accurate results
```diff
@@ Coverage Diff @@
## master #19696 +/- ##
==========================================
+ Coverage 66.51% 66.53% +0.01%
==========================================
Files 1684 1684
Lines 64559 64575 +16
Branches 6626 6636 +10
==========================================
+ Hits 42941 42963 +22
+ Misses 19923 19916 -7
- Partials 1695 1696 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.19% <68.25%> (+0.04%)` | :arrow_up: |
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/19696?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/19696/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.81% <0.00%> (+0.81%)` | :arrow_up: |
| [.../src/explore/components/ControlPanelsContainer.tsx](https://codecov.io/gh/apache/superset/pull/19696/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==) | `79.80% <ø> (ø)` | |
| [.../explore/components/ExploreViewContainer/index.jsx](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci9pbmRleC5qc3g=) | `54.01% <9.09%> (-0.89%)` | :arrow_down: |
| [...t-frontend/src/explore/components/ExploreAlert.tsx](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQWxlcnQudHN4) | `58.33% <58.33%> (ø)` | |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `78.49% <93.54%> (+4.88%)` | :arrow_up: |
| [...et-frontend/src/components/Chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnRSZW5kZXJlci5qc3g=) | `53.44% <100.00%> (+0.81%)` | :arrow_up: |
| [...rc/explore/controlUtils/getFormDataFromControls.ts](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzL2dldEZvcm1EYXRhRnJvbUNvbnRyb2xzLnRz) | `100.00% <100.00%> (ø)` | |
| [...frontend/src/SqlLab/components/ResultSet/index.tsx](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC9pbmRleC50c3g=) | `50.00% <0.00%> (-0.50%)` | :arrow_down: |
| [superset-frontend/src/explore/constants.ts](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29uc3RhbnRzLnRz) | `100.00% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/19696/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `33.15% <0.00%> (ø)` | |
| ... and [4 more](https://codecov.io/gh/apache/superset/pull/19696/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/19696?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/19696?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 [c8304a2...d6a2e0b](https://codecov.io/gh/apache/superset/pull/19696?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] kgabryje commented on pull request #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19696:
URL: https://github.com/apache/superset/pull/19696#issuecomment-1099088494
/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 a diff in pull request #19696: feat(explore): Replace overlay with alert banner when chart controls change
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #19696:
URL: https://github.com/apache/superset/pull/19696#discussion_r852725249
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -108,25 +107,23 @@ const Styles = styled.div`
}
`;
-const RefreshOverlayWrapper = styled.div`
- position: absolute;
- top: 0;
- left: 0;
- width: 100%;
- height: 100%;
- display: flex;
- align-items: center;
- justify-content: center;
-`;
-
const MonospaceDiv = styled.div`
font-family: ${({ theme }) => theme.typography.families.monospace};
- white-space: pre;
word-break: break-word;
overflow-x: auto;
white-space: pre-wrap;
`;
+const requiredFieldsMissingWarning = isFeatureEnabled(
+ FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP,
+)
+ ? t(
+ 'Drag and drop values into highlighted field(s) in the control panel. Then run the query by clicking on the "Create chart" button.',
+ )
+ : t(
+ 'Select values in highlighted field(s) in the control panel. Then run the query by clicking on the "Create chart" button.',
+ );
Review Comment:
Sounds good 👍
--
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