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/05/09 13:44:28 UTC
[GitHub] [superset] kgabryje opened a new pull request, #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
kgabryje opened a new pull request, #19993:
URL: https://github.com/apache/superset/pull/19993
### 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.
Currently, that modal is not customizable. However, when we implement Single Page Application in Superset, we'll probably be able to customize the modal when navigating within Superset.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<img width="1792" alt="image" src="https://user-images.githubusercontent.com/15073128/167422541-da6709f4-605b-4740-8cb5-80f693b956ea.png">
### TESTING INSTRUCTIONS
1. Open a chart
2. Make some changes in control panel
3. Try to leave the page (close tab or navigate to another page)
4. Verify that browser confirmation modal appears
1. Open a chart
2. Make some changes in control panel and save.
3. Leave the page, verify that the modal didn't appear
1. Open a chart
2. Make some changes in control panel.
3. Revert those changes (for example add and remove a metric)
4. Leave the page, verify that the modal didn't appear
### 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] codecov[bot] commented on pull request #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1121190065
# [Codecov](https://codecov.io/gh/apache/superset/pull/19993?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 [#19993](https://codecov.io/gh/apache/superset/pull/19993?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77c6098) into [master](https://codecov.io/gh/apache/superset/commit/c3ba86ecc5b687c10403d5b37fce2e398dea610d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c3ba86e) will **decrease** coverage by `0.00%`.
> The diff coverage is `82.05%`.
```diff
@@ Coverage Diff @@
## master #19993 +/- ##
==========================================
- Coverage 66.29% 66.29% -0.01%
==========================================
Files 1712 1712
Lines 63962 63985 +23
Branches 6731 6733 +2
==========================================
+ Hits 42404 42416 +12
- Misses 19847 19857 +10
- Partials 1711 1712 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.28% <82.05%> (+<0.01%)` | :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/19993?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../explore/components/ExploreViewContainer/index.jsx](https://codecov.io/gh/apache/superset/pull/19993/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=) | `53.19% <61.53%> (+0.62%)` | :arrow_up: |
| [...rset-frontend/src/explore/exploreUtils/formData.ts](https://codecov.io/gh/apache/superset/pull/19993/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) | `89.13% <92.00%> (+3.41%)` | :arrow_up: |
| [...-frontend/src/components/AlteredSliceTag/index.jsx](https://codecov.io/gh/apache/superset/pull/19993/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.23% <100.00%> (-1.01%)` | :arrow_down: |
| [...ugins/legacy-plugin-chart-calendar/src/Calendar.js](https://codecov.io/gh/apache/superset/pull/19993/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNhbGVuZGFyL3NyYy9DYWxlbmRhci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...gins/legacy-plugin-chart-world-map/src/WorldMap.js](https://codecov.io/gh/apache/superset/pull/19993/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LXdvcmxkLW1hcC9zcmMvV29ybGRNYXAuanM=) | `0.00% <0.00%> (ø)` | |
| [...s/plugin-chart-pivot-table/src/PivotTableChart.tsx](https://codecov.io/gh/apache/superset/pull/19993/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtcGl2b3QtdGFibGUvc3JjL1Bpdm90VGFibGVDaGFydC50c3g=) | `0.00% <0.00%> (ø)` | |
| [.../preset-chart-xy/src/components/BoxPlot/Encoder.ts](https://codecov.io/gh/apache/superset/pull/19993/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wcmVzZXQtY2hhcnQteHkvc3JjL2NvbXBvbmVudHMvQm94UGxvdC9FbmNvZGVyLnRz) | `0.00% <0.00%> (ø)` | |
| [...legacy-plugin-chart-calendar/src/ReactCalendar.jsx](https://codecov.io/gh/apache/superset/pull/19993/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWNhbGVuZGFyL3NyYy9SZWFjdENhbGVuZGFyLmpzeA==) | `0.00% <0.00%> (ø)` | |
| [...preset-chart-xy/src/components/BoxPlot/BoxPlot.tsx](https://codecov.io/gh/apache/superset/pull/19993/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wcmVzZXQtY2hhcnQteHkvc3JjL2NvbXBvbmVudHMvQm94UGxvdC9Cb3hQbG90LnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...egacy-plugin-chart-world-map/src/ReactWorldMap.jsx](https://codecov.io/gh/apache/superset/pull/19993/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LXdvcmxkLW1hcC9zcmMvUmVhY3RXb3JsZE1hcC5qc3g=) | `0.00% <0.00%> (ø)` | |
| ... and [4 more](https://codecov.io/gh/apache/superset/pull/19993/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/19993?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/19993?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 [c3ba86e...77c6098](https://codecov.io/gh/apache/superset/pull/19993?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 #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1121163148
@kgabryje If I do more than one change, then the confirmation is not appearing. The `beforeunload` is being unregistered.
https://user-images.githubusercontent.com/70410625/167429669-62e98c72-3812-485a-b74a-74d1795c4b8f.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 #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1122337875
Well spotted @michael-s-molina! The problem was that the cleanup function in use effect was being called on each change of its dependencies. I moved the cleanup to a separate useEffect that gets called only on unmount.
Also I fixed an old bug that caused the "Altered" pill to stay visible when there was an adhoc metric added and then you added some metric and deleted it immediately (so effectively there was no change, but "Altered" stayed). The reason for that was that in initial form data the adhoc metric is represented by a plain JSON object, but later it's transformed to AdhocMetric instance, so `isEqual` is false even if the field values are the same. I fixed it by omitting `__proto__` before calling `isEqual` so that we ignore the difference between plain object and class instance
--
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 #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1123584478
/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] github-actions[bot] commented on pull request #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1123821016
@kgabryje Ephemeral environment spinning up at http://52.12.219.96: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 pull request #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1123644883
Found two more problems:
1 - It's showing the confirmation when navigating using the main header without any changes.
2 - It's showing the confirmation when dropping a filter and canceling
https://user-images.githubusercontent.com/70410625/167842795-3dfbb17e-60ab-4919-b4bc-1626e14234db.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 #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1123723611
@michael-s-molina It looks like `sliceFormData` which was used as a reference for "initial" form data didn't include values of controls that are "trigger renders" (so mostly controls in Customize section). I fixed it by using actual initial form data instead of form data from chart.
The takeaway is that in case the user does the following steps:
1. Open a chart
2. Change something in form data
3. Run query
4. Reload the page (confirmation modal appears)
5. Close the page without saving - confirmation modal doesn't appear
On the other hand such flow might be expected - when user changes something in the chart and then sends a link to someone else, that other user shouldn't see the confirmation modal before closing the tab.
I think that current solution is optimal, but tagging @lauderbaugh @kasiazjc @rusackas in case you guys have different opinion.
--
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 #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1123808765
/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] github-actions[bot] commented on pull request #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1124822835
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 merged pull request #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje merged PR #19993:
URL: https://github.com/apache/superset/pull/19993
--
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 #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1123586668
@kgabryje Ephemeral environment spinning up at http://54.244.56.255: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] kgabryje commented on pull request #19993: feat(explore): Show confirmation modal if user exits Explore without saving changes
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #19993:
URL: https://github.com/apache/superset/pull/19993#issuecomment-1123710841
> Found two more problems: 1 - It's showing the confirmation when navigating using the main header without any changes. 2 - It's showing the confirmation when dropping a filter and canceling
>
> Screen.Recording.2022-05-11.at.8.47.13.AM.mov
Thanks for more testing 🙂
1. It looks like the algorithm for calculating diffs for `Altered` table is faulty, because the confirmation modal uses the same logic. `Altered` pill probably shouldn't popup here as you just opened a saved chart, so the problem might be related. Will investigate.
2. I don't think it's related to dropping and cancelling filter. The `Altered` pill is visible and it's most likely that that's why the confirmation modal is showing.
--
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