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/07/28 13:57:41 UTC
[GitHub] [superset] kgabryje opened a new pull request, #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
kgabryje opened a new pull request, #20902:
URL: https://github.com/apache/superset/pull/20902
### SUMMARY
This PR adds a browser confirmation modal (similar to the one that we have in dashboard edit mode) when user makes a change in control panel and then tries to close Explore (either by closing the tab or moving to another page) without clicking Save button.
The modal that displays when user navigates out of SPA context (some external page or a Superset page that hasn't been migrated to SPA yet) or closes the tab, is not customizable.
In the case of navigating within SPA context, we can easily display a custom message in the modal thanks to react-router's `Prompt` component. However, displaying a custom modal instead of the default one rendered by `<Prompt />` requires more effort and will be handled in a separate PR.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
tbd
### TESTING INSTRUCTIONS
tbd
### 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
--
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 closed pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina closed pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
URL: https://github.com/apache/superset/pull/20902
--
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] codyml commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1205393332
> > > > I think it makes sense! @kasiazjc wdyt?
> > >
> > >
> > > Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one. Copy:
> > > Your chart is not saved If you leave and don't save, changes will be lost. [Cancel][Leave]
> > > Thoughts?
> >
> >
> > Do we have a final plan for this design? My personal feeling is will be great to have all, [Cancel][Leave][Save], if that is too complicated, i think i will vote for [Cancel][Leave] which is consistent with the chrome flow, and i can always cancel and do save on explore.
>
> I try to avoid adding 3 buttons as much as possible, as it can get quite confusing. After clicking "save" we would still have to open "save" modal, so I would say let's go with [Cancel][Leave] @kgabryje
[Cancel] [Leave] makes the most sense to me too! As a user I think I'd expect there to be an option to just cancel the close.
--
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] codyml commented on a diff in pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20902:
URL: https://github.com/apache/superset/pull/20902#discussion_r964509182
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -337,7 +375,7 @@ function ExploreViewContainer(props) {
}
function toggleModal() {
- setShowingModal(!showingModal);
+ props.actions.setSaveModalVisible(!props.isSaveModalVisible);
Review Comment:
I also noticed that if I Save As and it doesn't refresh, it'll then show the modal every time even if I save it again.
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199676195
> @kgabryje I'm not sure if this is fixable or if it was like this before, but I noticed that if you have unsaved changes and try to navigate to Dashboard, even if you click "cancel" the navigation indicator stays on Dashboard
That's an existing bug - you can try it by going to charts list, then, dashboards list, then click "back" - dashboards will still be highlighted
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1200799746
@jinghua-qa Ephemeral environment spinning up at http://18.236.190.110: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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1237981850
> Updated with Cancel/Leave
>
> <img alt="image" width="1792" src="https://user-images.githubusercontent.com/15073128/188612901-f83f9f43-674a-4c21-a8e0-d179578bfda2.png">
lgtm, thankss
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199000428
/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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199676804
> @kgabryje Also not sure if this was intentional, but I noticed that if you edit the chart title but clicking directly on the rendered title and edit it, the "Altered" tag appears but it doesn't block navigation away.
Oops, well spotted!
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20902:
URL: https://github.com/apache/superset/pull/20902#discussion_r933437011
##########
superset-frontend/src/components/RouteLeaveGuard/RouteLeaveGuard.tsx:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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, { useEffect, useRef } from 'react';
+import { Prompt } from 'react-router-dom';
+// eslint-disable-next-line import/no-extraneous-dependencies
+import { PromptProps } from 'react-router';
+
+const handleUnloadEvent = (e: BeforeUnloadEvent) => {
Review Comment:
Good point!
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1200837996
> I also noticed that there are different confirmation dialog, and this one did not have save option, only cancel and leave, is this expected? are we trying to have same confirmation dialog modal when user leave explore without save? <img alt="Screen Shot 2022-08-01 at 12 33 52 AM" width="1792" src="https://user-images.githubusercontent.com/81597121/182097434-a235c2ed-268c-4724-87ff-0ab6f7a707d2.png">
Yes - our custom confirmation modal is possible to display only when navgating between SPA routes. When you try to close the tab or go to an external page, a default browser modal appears - it's not customizable in any way (it's a browser security measure)
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1249920330
/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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1205032503
> > > I think it makes sense! @kasiazjc wdyt?
> >
> >
> > Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one. Copy:
> > Your chart is not saved If you leave and don't save, changes will be lost. [Cancel][Leave]
> > Thoughts?
>
> Do we have a final plan for this design? My personal feeling is will be great to have all, [Cancel][Leave][Save], if that is too complicated, i think i will vote for [Cancel][Leave] which is consistent with the chrome flow, and i can always cancel and do save on explore.
I try to avoid adding 3 buttons as much as possible, as it can get quite confusing. After clicking "save" we would still have to open "save" modal, so I would say let's go with [Cancel][Leave] @kgabryje
--
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] codyml commented on a diff in pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20902:
URL: https://github.com/apache/superset/pull/20902#discussion_r933347007
##########
superset-frontend/src/components/RouteLeaveGuard/RouteLeaveGuard.tsx:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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, { useEffect, useRef } from 'react';
+import { Prompt } from 'react-router-dom';
+// eslint-disable-next-line import/no-extraneous-dependencies
+import { PromptProps } from 'react-router';
+
+const handleUnloadEvent = (e: BeforeUnloadEvent) => {
Review Comment:
If someone tries to reuse this component to guard against route changes at two places in the same tree, I think there might be unexpected behavior because all `addEventListener`/`removeEventListener` calls from different instances of the component will reference the same function. How about moving it inside the component body so it's re-defined for each instantiation?
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1200836336
> in the confirmation modal, is it make sense to have a cancel option or close dialog option? Looks like now only have discard and save, as user i may need to think about double check my change before i save, but i dont have an option currenlty. <img alt="Screen Shot 2022-08-01 at 12 29 50 AM" width="1789" src="https://user-images.githubusercontent.com/81597121/182096519-363d7101-a2ca-4ccf-9811-6d80e0a77301.png">
I think it makes sense! @kasiazjc wdyt?
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1204512973
> > I think it makes sense! @kasiazjc wdyt?
>
> Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one. Copy:
>
> Your chart is not saved If you leave and don't save, changes will be lost. [Cancel][Leave]
>
> Thoughts?
Do we have a final plan for this design? My personal feeling is will be great to have all, [Cancel][Leave][Save], if that is too complicated, i think i will vote for [Cancel][Leave] which is consistent with the chrome flow, and i can always cancel and do save on explore.
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1249921959
@jinghua-qa Ephemeral environment spinning up at http://34.211.247.176: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] codyml commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199474313
@kgabryje Also not sure if this was intentional, but I noticed that if you edit the chart title but clicking directly on the rendered title and edit it, the "Altered" tag appears but it doesn't block navigation away
--
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] codyml commented on a diff in pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20902:
URL: https://github.com/apache/superset/pull/20902#discussion_r964464022
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -337,7 +375,7 @@ function ExploreViewContainer(props) {
}
function toggleModal() {
- setShowingModal(!showingModal);
+ props.actions.setSaveModalVisible(!props.isSaveModalVisible);
Review Comment:
I think this PR might be re-triggering the annoying bug that forced us to [revert these two PRs](https://github.com/apache/superset/pull/20796):
https://user-images.githubusercontent.com/13007381/188811425-a306f8b2-4ec9-4aa1-94f7-504759ea1a6f.mov
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1237966918
Updated with Cancel/Leave
<img width="1792" alt="image" src="https://user-images.githubusercontent.com/15073128/188612901-f83f9f43-674a-4c21-a8e0-d179578bfda2.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] jinghua-qa commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1200794979
/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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199674819
> @kgabryje Also not sure if this was intentional, but I noticed that if you edit the chart title but clicking directly on the rendered title and edit it, the "Altered" tag appears but it doesn't block navigation away.
That's an existing bug - you can try it by going to charts list, then, dashboards list, then click "back" - dashboards will still be highlighted
--
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 #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199042374
@jinghua-qa Ephemeral environment spinning up at http://35.86.174.73: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] codecov[bot] commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1198380870
# [Codecov](https://codecov.io/gh/apache/superset/pull/20902?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 [#20902](https://codecov.io/gh/apache/superset/pull/20902?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2509d5a) into [master](https://codecov.io/gh/apache/superset/commit/718bc3062e99cc44afbb57f786b5ca228c5b13fb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (718bc30) will **increase** coverage by `0.03%`.
> The diff coverage is `69.23%`.
```diff
@@ Coverage Diff @@
## master #20902 +/- ##
==========================================
+ Coverage 66.27% 66.31% +0.03%
==========================================
Files 1758 1759 +1
Lines 67073 67000 -73
Branches 7122 7097 -25
==========================================
- Hits 44453 44430 -23
+ Misses 20801 20754 -47
+ Partials 1819 1816 -3
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `52.00% <69.23%> (+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/20902?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rset-frontend/src/explore/components/SaveModal.tsx](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwudHN4) | `36.47% <0.00%> (ø)` | |
| [...ols/DndColumnSelectControl/ColumnSelectPopover.tsx](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL0NvbHVtblNlbGVjdFBvcG92ZXIudHN4) | `3.37% <ø> (+0.10%)` | :arrow_up: |
| [...src/components/RouteLeaveGuard/RouteLeaveGuard.tsx](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvUm91dGVMZWF2ZUd1YXJkL1JvdXRlTGVhdmVHdWFyZC50c3g=) | `50.00% <50.00%> (ø)` | |
| [.../explore/components/ExploreViewContainer/index.jsx](https://codecov.io/gh/apache/superset/pull/20902/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=) | `52.38% <57.14%> (+0.18%)` | :arrow_up: |
| [...rset-frontend/src/explore/exploreUtils/formData.ts](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzL2Zvcm1EYXRhLnRz) | `88.88% <91.66%> (+3.17%)` | :arrow_up: |
| [...-frontend/src/components/AlteredSliceTag/index.jsx](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnL2luZGV4LmpzeA==) | `87.75% <100.00%> (-0.82%)` | :arrow_down: |
| [...rontend/src/components/ListView/Filters/Select.tsx](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvRmlsdGVycy9TZWxlY3QudHN4) | `52.38% <0.00%> (-2.62%)` | :arrow_down: |
| [...et-frontend/src/components/TableSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGFibGVTZWxlY3Rvci9pbmRleC50c3g=) | `76.08% <0.00%> (ø)` | |
| [...t-frontend/src/filters/components/GroupBy/types.ts](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9Hcm91cEJ5L3R5cGVzLnRz) | `100.00% <0.00%> (ø)` | |
| [...frontend/src/components/DatabaseSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/20902/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGF0YWJhc2VTZWxlY3Rvci9pbmRleC50c3g=) | `88.88% <0.00%> (ø)` | |
| ... and [4 more](https://codecov.io/gh/apache/superset/pull/20902/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) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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] eschutho commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199810061
@jinghua-qa can we test what happens when the chart is running on a query? Does it show the correct save chart modal with the dialog to save a dataset?
--
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] codyml commented on a diff in pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20902:
URL: https://github.com/apache/superset/pull/20902#discussion_r964467771
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -337,7 +375,7 @@ function ExploreViewContainer(props) {
}
function toggleModal() {
- setShowingModal(!showingModal);
+ props.actions.setSaveModalVisible(!props.isSaveModalVisible);
Review Comment:
I don't think we were able to solve it: IIRC it seemed to happen whenever we tried to send additional Redux actions at the same time as when Explore was rendering. [Here's the PR](https://github.com/apache/superset/pull/20704) that would have fixed it if we hadn't reverted the whole thing. Not sure if your case is similar? I think that also might be why Cypress is failing on yours.
--
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] codyml commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1199468629
@kgabryje I'm not sure if this is fixable or if it was like this before, but I noticed that if you have unsaved changes and try to navigate to Dashboard, even if you click "cancel" the navigation indicator stays on Dashboard:
<img width="472" alt="Screen Shot 2022-07-29 at 10 58 12 AM" src="https://user-images.githubusercontent.com/13007381/181787782-21d075d0-c1e3-4a89-b1a2-b32432fc90cd.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] jinghua-qa commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1200827067
in the confirmation modal, is it make sense to have a cancel option or close dialog option? Looks like now only have discard and save, as user i may need to think about double check my change before i save, but i dont have an option currenlty.
<img width="1789" alt="Screen Shot 2022-08-01 at 12 29 50 AM" src="https://user-images.githubusercontent.com/81597121/182096519-363d7101-a2ca-4ccf-9811-6d80e0a77301.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] jinghua-qa commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1200830737
I also noticed that there are different confirmation dialog, and this one did not have save option, only cancel and leave, is this expected? are we trying to have same confirmation dialog modal when user leave explore without save?
<img width="1792" alt="Screen Shot 2022-08-01 at 12 33 52 AM" src="https://user-images.githubusercontent.com/81597121/182097434-a235c2ed-268c-4724-87ff-0ab6f7a707d2.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] kasiazjc commented on pull request #20902: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #20902:
URL: https://github.com/apache/superset/pull/20902#issuecomment-1201270601
> > in the confirmation modal, is it make sense to have a cancel option or close dialog option? Looks like now only have discard and save, as user i may need to think about double check my change before i save, but i dont have an option currenlty. <img alt="Screen Shot 2022-08-01 at 12 29 50 AM" width="1789" src="https://user-images.githubusercontent.com/81597121/182096519-363d7101-a2ca-4ccf-9811-6d80e0a77301.png">
>
> I think it makes sense! @kasiazjc wdyt?
Good point, I'm thinking - maybe for consistency we should follow chrome flow actually instead of adding this one.
Copy:
Your chart is not saved
If you leave and don't save, changes will be lost.
[Cancel][Leave]
Thoughts?
--
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