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 2020/05/16 05:58:51 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #9817: [explore view] inline edit slice name should not overwrite
graceguo-supercat opened a new pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Currently in explore view, inline override chart title without confirmation behavior is very confusing:
1. user open a chart
2. as long as he/she is the chart owner, he/she can click the chart title, and inline edit chart title. Superset will automatically overwrite new chart title in the database, without user clicking a confirm button. User may not realize the original chart title is overwritten.
3. if user wants to make a copy of the original chart, they may edit title first then click Save button and choose "Save as". But at this moment the original title is already changed.
Proposed solution:
1. user can still inline edit chart title, but Superset won't overwrite chart title right away. The changes is only in front-end, not change backend slice data.
2. Only when user click explore view's Save button, and choose save as or overwrite, the new title will be saved with other changes. If user choose save as, original chart title won't be updated.
### TEST PLAN
CI and manual test
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] 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.
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] [incubator-superset] codecov-io commented on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-629594406
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=h1) Report
> Merging [#9817](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ea9b7f2dc426cf912f8f5f2b749cc707c9af6964&el=desc) will **increase** coverage by `4.63%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9817 +/- ##
==========================================
+ Coverage 66.16% 70.79% +4.63%
==========================================
Files 585 184 -401
Lines 30427 17863 -12564
Branches 3152 0 -3152
==========================================
- Hits 20133 12647 -7486
+ Misses 10113 5216 -4897
+ Partials 181 0 -181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `?` | |
| #python | `70.79% <ø> (-0.23%)` | :arrow_down: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `58.92% <0.00%> (-21.43%)` | :arrow_down: |
| [superset/utils/cache.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `45.83% <0.00%> (-20.84%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.26% <0.00%> (-13.05%)` | :arrow_down: |
| [superset/views/database/api.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvYXBpLnB5) | `83.90% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `97.29% <0.00%> (-2.71%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `83.77% <0.00%> (-2.36%)` | :arrow_down: |
| [superset/jinja\_context.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvamluamFfY29udGV4dC5weQ==) | `84.69% <0.00%> (-1.03%)` | :arrow_down: |
| [superset/security/manager.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | `88.62% <0.00%> (-0.35%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.90% <0.00%> (-0.23%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.41% <0.00%> (-0.16%)` | :arrow_down: |
| ... and [394 more](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=footer). Last update [ea9b7f2...9a1ecbe](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630393813
My point is this quick save makes Save behavior inconsistent.
- _Auto-save_ is a standard behavior for many application. If we want to offer auto-save, the behavior should be consistent, all the changes should have auto-saved.
- _Click-and-save_ is another set of standard behavior, it also used by many applications. But mix this 2 may make user will feel sometimes Superset can auto save their change without they click anything, but sometimes not. Only developer read the code will find out the subtle difference.
- Even we offer auto-save, user may still feel uncertain after they have all the edit work done. So they will still want to click save button to re-assure all their work is saved. So the quick auto-save a middle copy is not necessary.
The inconsistency will bring more confusion than very little connivence.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630393813
My point is this quick save makes Save behavior inconsistent.
- _Auto-save_ is a standard behavior for many application. If we want to offer auto-save, the behavior should be consistent, all the changes should have auto-saved.
- _Click-and-save_ is another set of standard behavior, it also used by many applications. But mix this 2 may make user will feel sometimes Superset can auto save their change without they click anything, but sometimes not. Only developer read the code will find out the subtle difference.
The inconsistency will bring more confusion than very little connivence.
----------------------------------------------------------------
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.
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] [incubator-superset] etr2460 commented on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-629688078
omg, thank you so much for fixing this. i've made this mistake sooo many times.
one consideration, do you think the behavior should be different on the initial naming of the chart? So if the user changes the name from `- untitled` to something, then we treat that as the first time saving the chart and write to the backend, but any other time we don't overwrite the name until the user explicitly clicks the save button.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat merged pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817
----------------------------------------------------------------
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.
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] [incubator-superset] ktmud commented on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630593453
I'm wondering whether we can just remove inline edit? There is an edit button/icon anyway, 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.
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] [incubator-superset] graceguo-supercat commented on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630594291
_inline edit_ feature is also requested by users :)
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-629594406
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=h1) Report
> Merging [#9817](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ea9b7f2dc426cf912f8f5f2b749cc707c9af6964&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9817 +/- ##
==========================================
- Coverage 66.16% 66.16% -0.01%
==========================================
Files 585 585
Lines 30427 30408 -19
Branches 3152 3149 -3
==========================================
- Hits 20133 20120 -13
+ Misses 10113 10107 -6
Partials 181 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `59.24% <66.66%> (-0.02%)` | :arrow_down: |
| #python | `71.02% <ø> (ø)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9leHBsb3JlQWN0aW9ucy5qcw==) | `34.32% <0.00%> (ø)` | |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `11.11% <ø> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `46.47% <ø> (ø)` | |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `25.45% <ø> (+0.45%)` | :arrow_up: |
| [...rset-frontend/src/explore/components/SaveModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwuanN4) | `91.39% <75.00%> (-1.24%)` | :arrow_down: |
| [...tend/src/explore/components/ExploreChartHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIuanN4) | `65.62% <100.00%> (+0.31%)` | :arrow_up: |
| [...t-frontend/src/explore/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZ2V0SW5pdGlhbFN0YXRlLmpz) | `91.66% <100.00%> (+0.75%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=footer). Last update [ea9b7f2...9a1ecbe](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630594291
_inline edit_ feature is also requested by users :) But it seems hard to decide overwrite or save_as
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on a change in pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#discussion_r427040230
##########
File path: superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx
##########
@@ -65,24 +65,9 @@ describe('ExploreChartHeader', () => {
expect(wrapper.find(ExploreActionButtons)).toHaveLength(1);
});
- it('should updateChartTitleOrSaveSlice for existed slice', () => {
+ it('should update title but not save', () => {
const newTitle = 'New Chart Title';
- wrapper.instance().updateChartTitleOrSaveSlice(newTitle);
- expect(stub.call).toHaveLength(1);
- expect(stub).toHaveBeenCalledWith(mockProps.slice.form_data, {
- action: 'overwrite',
- slice_name: newTitle,
- });
- });
-
- it('should updateChartTitleOrSaveSlice for new slice', () => {
- const newTitle = 'New Chart Title';
- wrapper.setProps({ slice: undefined });
- wrapper.instance().updateChartTitleOrSaveSlice(newTitle);
- expect(stub.call).toHaveLength(1);
- expect(stub).toHaveBeenCalledWith(mockProps.form_data, {
- action: 'saveas',
- slice_name: newTitle,
- });
+ const editableTitle = wrapper.find(EditableTitle);
+ expect(editableTitle.props().onSaveTitle).toBe(updateChartTitleStub);
Review comment:
this line is to make sure child component EditableTitle will call updateChartTitle for after title is changed.
In the old code, it was to make sure `saveSlice` was called by callback function. In the new implementation, `updateChartTitle` is callback function.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630594291
_inline edit_ feature is also requested by users :) But it seems hard to decide overwrite or save_as.
----------------------------------------------------------------
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.
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] [incubator-superset] etr2460 commented on a change in pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#discussion_r426979019
##########
File path: superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx
##########
@@ -65,24 +65,9 @@ describe('ExploreChartHeader', () => {
expect(wrapper.find(ExploreActionButtons)).toHaveLength(1);
});
- it('should updateChartTitleOrSaveSlice for existed slice', () => {
+ it('should update title but not save', () => {
const newTitle = 'New Chart Title';
- wrapper.instance().updateChartTitleOrSaveSlice(newTitle);
- expect(stub.call).toHaveLength(1);
- expect(stub).toHaveBeenCalledWith(mockProps.slice.form_data, {
- action: 'overwrite',
- slice_name: newTitle,
- });
- });
-
- it('should updateChartTitleOrSaveSlice for new slice', () => {
- const newTitle = 'New Chart Title';
- wrapper.setProps({ slice: undefined });
- wrapper.instance().updateChartTitleOrSaveSlice(newTitle);
- expect(stub.call).toHaveLength(1);
- expect(stub).toHaveBeenCalledWith(mockProps.form_data, {
- action: 'saveas',
- slice_name: newTitle,
- });
+ const editableTitle = wrapper.find(EditableTitle);
+ expect(editableTitle.props().onSaveTitle).toBe(updateChartTitleStub);
Review comment:
can we add to this test `expect(saveSliceStub.call).toHaveLength(0)` or something like that to verify that the chart wasn't saved?
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630371062
The old implementation can save new chart when user give `untitled` new chart a title. But I feel this behavior is also confusing:
- when try to save `untitled` chart, do we save chart title only, or should also save chart's query parameter,
- when user keep exploring this new chart and changed control, do they know they need to save again?
I feel the quick save behavior might bring more confusion than convenience, so i would like to keep a consistent behavior: _**nothing will be saved until user clicks save button**_.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630393813
My point is this quick save makes Save behavior inconsistent.
Auto save is a standard behavior for many application. If we want to offer auto-save, the behavior should be consistent, all the changes should have auto-saved.
Click and save is another set of standard behavior, it also used by many applications. But mix this 2 may make user will feel sometimes Superset can auto save their change without they click anything, but sometimes not. Only developer read the code will find out the subtle difference.
The inconsistency will bring more confusion than very little connivence.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630393813
My point is this quick save makes Save behavior inconsistent.
- _Auto-save_ is a standard behavior for many application. If we want to offer auto-save, the behavior should be consistent, all the changes should have auto-saved.
- _Click-and-save_ is another set of standard behavior, it also used by many applications. But mix this 2 may make user will feel sometimes Superset can auto save their change without they click anything, but sometimes not. Only developer read the code will find out the subtle difference.
- Even we offer quiet auto-save, user may still feel uncertain after they have all the edit work done. So they will still want to click save button to re-assure all their work is saved. So the quick auto-save a middle copy is not necessary.
The inconsistency will bring more confusion than very little connivence.
----------------------------------------------------------------
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.
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] [incubator-superset] etr2460 commented on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630386661
to answer your questions:
1. When you first name a chart, we save the current state of everything to a new chart with that name. This is a pretty standard use case, e.g. I set up a chart to how i like it, name it, then send it to someone/add it to a dashboard.
2. I think because we have the `altered` label, it should be clear enough to the user when they've made some changes that aren't saved
I think we should be careful about breaking the happy path here and only address the problem cases. The problem here is that changing a chart title instantly saves and overwrites the chart. If the chart was never saved before, then there's nothing to overwrite, so autosaving on the first title change doesn't seem too inconsistent/messy to me
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-629594406
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=h1) Report
> Merging [#9817](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ea9b7f2dc426cf912f8f5f2b749cc707c9af6964&el=desc) will **decrease** coverage by `0.13%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9817 +/- ##
==========================================
- Coverage 66.16% 66.03% -0.14%
==========================================
Files 585 585
Lines 30427 30408 -19
Branches 3152 3149 -3
==========================================
- Hits 20133 20079 -54
- Misses 10113 10148 +35
Partials 181 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `59.24% <66.66%> (-0.02%)` | :arrow_down: |
| #python | `70.79% <ø> (-0.23%)` | :arrow_down: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9leHBsb3JlQWN0aW9ucy5qcw==) | `34.32% <0.00%> (ø)` | |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `11.11% <ø> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `46.47% <ø> (ø)` | |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `25.45% <ø> (+0.45%)` | :arrow_up: |
| [...rset-frontend/src/explore/components/SaveModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwuanN4) | `91.39% <75.00%> (-1.24%)` | :arrow_down: |
| [...tend/src/explore/components/ExploreChartHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIuanN4) | `65.62% <100.00%> (+0.31%)` | :arrow_up: |
| [...t-frontend/src/explore/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZ2V0SW5pdGlhbFN0YXRlLmpz) | `91.66% <100.00%> (+0.75%)` | :arrow_up: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `58.92% <0.00%> (-21.43%)` | :arrow_down: |
| [superset/utils/cache.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `45.83% <0.00%> (-20.84%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.26% <0.00%> (-13.05%)` | :arrow_down: |
| ... and [7 more](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=footer). Last update [ea9b7f2...9a1ecbe](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630393813
My point is this quick save makes Save behavior inconsistent.
_Auto-save_ is a standard behavior for many application. If we want to offer auto-save, the behavior should be consistent, all the changes should have auto-saved.
_Click-and-save_ is another set of standard behavior, it also used by many applications. But mix this 2 may make user will feel sometimes Superset can auto save their change without they click anything, but sometimes not. Only developer read the code will find out the subtle difference.
The inconsistency will bring more confusion than very little connivence.
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-629594406
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=h1) Report
> Merging [#9817](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ea9b7f2dc426cf912f8f5f2b749cc707c9af6964&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9817 +/- ##
==========================================
- Coverage 66.16% 66.12% -0.05%
==========================================
Files 585 585
Lines 30427 30408 -19
Branches 3152 3149 -3
==========================================
- Hits 20133 20108 -25
- Misses 10113 10119 +6
Partials 181 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `59.24% <66.66%> (-0.02%)` | :arrow_down: |
| #python | `70.96% <ø> (-0.07%)` | :arrow_down: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9leHBsb3JlQWN0aW9ucy5qcw==) | `34.32% <0.00%> (ø)` | |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `11.11% <ø> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `46.47% <ø> (ø)` | |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `25.45% <ø> (+0.45%)` | :arrow_up: |
| [...rset-frontend/src/explore/components/SaveModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwuanN4) | `91.39% <75.00%> (-1.24%)` | :arrow_down: |
| [...tend/src/explore/components/ExploreChartHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIuanN4) | `65.62% <100.00%> (+0.31%)` | :arrow_up: |
| [...t-frontend/src/explore/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZ2V0SW5pdGlhbFN0YXRlLmpz) | `91.66% <100.00%> (+0.75%)` | :arrow_up: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.26% <0.00%> (-13.05%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `85.25% <0.00%> (-0.89%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.90% <0.00%> (-0.23%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=footer). Last update [ea9b7f2...9a1ecbe](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630594291
_inline edit_ feature is also requested by users :) I remembered the initial request is mean to overwrite without confirmation (quick save).
But it breaks many users only want save_as.
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-629594406
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=h1) Report
> Merging [#9817](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ea9b7f2dc426cf912f8f5f2b749cc707c9af6964&el=desc) will **increase** coverage by `4.52%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9817 +/- ##
==========================================
+ Coverage 66.16% 70.68% +4.52%
==========================================
Files 585 585
Lines 30427 30408 -19
Branches 3152 3149 -3
==========================================
+ Hits 20133 21495 +1362
+ Misses 10113 8795 -1318
+ Partials 181 118 -63
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `52.92% <66.66%> (?)` | |
| #javascript | `59.24% <66.66%> (-0.02%)` | :arrow_down: |
| #python | `71.02% <ø> (ø)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9leHBsb3JlQWN0aW9ucy5qcw==) | `44.77% <0.00%> (+10.44%)` | :arrow_up: |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `72.22% <ø> (+61.11%)` | :arrow_up: |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `77.46% <ø> (+30.98%)` | :arrow_up: |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `30.90% <ø> (+5.90%)` | :arrow_up: |
| [...rset-frontend/src/explore/components/SaveModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwuanN4) | `93.54% <75.00%> (+0.91%)` | :arrow_up: |
| [...tend/src/explore/components/ExploreChartHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIuanN4) | `81.25% <100.00%> (+15.94%)` | :arrow_up: |
| [...t-frontend/src/explore/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZ2V0SW5pdGlhbFN0YXRlLmpz) | `100.00% <100.00%> (+9.09%)` | :arrow_up: |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `61.13% <0.00%> (+0.65%)` | :arrow_up: |
| [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `53.84% <0.00%> (+1.28%)` | :arrow_up: |
| [superset-frontend/src/components/EditableTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRWRpdGFibGVUaXRsZS5qc3g=) | `81.69% <0.00%> (+1.40%)` | :arrow_up: |
| ... and [140 more](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=footer). Last update [ea9b7f2...9a1ecbe](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630371062
The old implementation can save new chart when user give `untitled` new chart a title. But I feel this behavior is also confusing:
- when try to auto save `untitled` chart, do we save chart title only, or should also save chart's query parameter,
- when user keep exploring this new chart and changed control, do they know they need to save again?
I feel the quick save behavior might bring more confusion than convenience, so i would like to keep a consistent behavior: _**nothing will be saved until user clicks save button**_.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630594291
_inline edit_ feature is also requested by users :) I remembered the initial request is mean to overwrite without confirmation (quick save).
But it seems it breaks many users only want save_as.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-630393813
My point is this quick save makes Save behavior inconsistent.
Auto save is a standard behavior for many application. If we want to offer auto-save, the behavior should be consistent, all the changes should have auto-saved.
Click and save is another set of standard behavior, it also used by many application. But mix this 2 may make user will feel sometimes Superset can auto save their change without they click anything, but sometimes not. Only developer read the code will find out the subtle difference.
The inconsistency will bring more confusion than very little connivence.
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #9817: fix: [explore view] inline edit slice name should not overwrite
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9817:
URL: https://github.com/apache/incubator-superset/pull/9817#issuecomment-629594406
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=h1) Report
> Merging [#9817](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ea9b7f2dc426cf912f8f5f2b749cc707c9af6964&el=desc) will **increase** coverage by `4.72%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9817 +/- ##
==========================================
+ Coverage 66.16% 70.88% +4.72%
==========================================
Files 585 585
Lines 30427 30408 -19
Branches 3152 3149 -3
==========================================
+ Hits 20133 21556 +1423
+ Misses 10113 8740 -1373
+ Partials 181 112 -69
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `53.71% <66.66%> (?)` | |
| #javascript | `59.24% <66.66%> (-0.02%)` | :arrow_down: |
| #python | `71.02% <ø> (ø)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9leHBsb3JlQWN0aW9ucy5qcw==) | `44.77% <0.00%> (+10.44%)` | :arrow_up: |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `72.22% <ø> (+61.11%)` | :arrow_up: |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `77.46% <ø> (+30.98%)` | :arrow_up: |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `30.90% <ø> (+5.90%)` | :arrow_up: |
| [...rset-frontend/src/explore/components/SaveModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwuanN4) | `93.54% <75.00%> (+0.91%)` | :arrow_up: |
| [...tend/src/explore/components/ExploreChartHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIuanN4) | `81.25% <100.00%> (+15.94%)` | :arrow_up: |
| [...t-frontend/src/explore/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZ2V0SW5pdGlhbFN0YXRlLmpz) | `100.00% <100.00%> (+9.09%)` | :arrow_up: |
| [...rontend/src/SqlLab/components/AceEditorWrapper.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIudHN4) | `56.98% <0.00%> (+1.07%)` | :arrow_up: |
| [superset-frontend/src/components/EditableTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRWRpdGFibGVUaXRsZS5qc3g=) | `81.69% <0.00%> (+1.40%)` | :arrow_up: |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `36.36% <0.00%> (+1.51%)` | :arrow_up: |
| ... and [142 more](https://codecov.io/gh/apache/incubator-superset/pull/9817/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=footer). Last update [ea9b7f2...9a1ecbe](https://codecov.io/gh/apache/incubator-superset/pull/9817?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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