-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(dashboard): Open "View Chart in Explore" in the same window #14778
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14778 +/- ##
=======================================
Coverage 77.57% 77.57%
=======================================
Files 963 963
Lines 49290 49290
Branches 6205 6205
=======================================
Hits 38235 38235
Misses 10854 10854
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@junlincc Ephemeral environment spinning up at http://34.218.44.244:8080. Credentials are |
Thanks for the PR. Please see video of Before and After behavior in select of Dashboard->Explore->Dashboard flow Screen.Recording.2021-05-26.at.10.53.26.AM.movPrevious behavior. View chart in Explore takes user to Explore, user revise the chart in Explore and can save it back to same dashboard( preselect in the dropdown). i dont think your PR changed the behavior, just wonder why the source dashboard is not preselected anymore. |
@junlincc I don't see the problem that you are describing but I see a worse problem. If you open up any dashboard it will always pre-select the same dashboard, independently from the actual dashboard that you have opened. This is not related to this PR as this problem is reproducible on master and in this branch also now that I have merged it with master. See video. COVID.Vaccine.Da.2.mp4Please let me know if you can reproduce it, we should file a separate issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I looked at the original PR, and I didn't see an explicit mention of meaning to start opening the browser in _blank
, so I assume that was a regression. But it would be nice if @graceguo-supercat could confirm this. However, what does seem to be intentional is targeting _blank
when pressing "Run in SQL Lab" from "Explore". I wonder if that behavior should also be changed, or if it's the exception that validates the rule?
Ephemeral environment shutdown and build artifacts deleted. |
Apologies for the late response. @geido @villebro et al. could one provide context as why we want to explore a chart in place as opposed to in a new tab? I think many users prefer opening a chart in a new tab whilst maintaining reference to the dashboard—especially when one wishes to open multiple charts (which isn't possible with this change). Additionally per @villebro's comment the behavior of opening a chart in a new tab is consistent with "Run in SQL Lab" etc. |
I didn't notice this PR till today....Airbnb users preferred to open "View Chart in Explore", because our dashboard is large, many charts, many tabs. A user may open a dashboard, then click a few tabs and nested sub tabs, and then click View chart. Open the chart in the same window will make user lost their context. Please revert this PR. I would like to know what's the benefit to open chart in the same window? |
@graceguo-supercat @john-bodley @junlincc The reason for the change was that users were annoyed by the proliferation of new tabs, when they felt they weren't necessary. Rather than reverting the PR, how about one of two means to support either behavior?
If neither of those work, or take too long, I suppose we can revert it and then carry it forward in a new PR if needed. |
@rusackas we think option (2) makes the most sense, though given this is a breaking change (in terms of UX) we sense it is best to revert the PR and in a follow up PR and the configuration option. |
This reverts commit faae27b.
hi @john-bodley @michellethomas i saw that you have reverted this change from locally. thanks for the feedback 🙏 We assume that the reason why a user click "View chart in Explore" is to tweak a specific chart, because all of the "view only" functionalities, including 1) view query 2)view chart details by hovering 3) maximize chart 4) filtering chart can all be done straightly from Dashboard. I apologize if this assumption doesn't make sense to your users and the inconvenience we introduced by the change.
User may take a while to adapt to the optimized new flow, but the original functionality - opening Explore was not removed by this PR. I would suggest to go with proposed solution 2 without reverting from the main repo. 🙏 |
Isn't this showing how frequently user needs to dig into many charts from a dashboard? If this is the case,
To me the option 2 is much easier and intuitive way. |
really BAD idea. don't do it. |
sure!! 😁 |
Totally agree make Superset more configurable and general |
(cherry picked from commit faae27b)
Thanks @rusackas for reverting this. In terms of user feedback, for this week's internal Airbnb release we forgot to locally revert the change (as was done in the previous release) and were pinged almost immediately by a user in Slack with the following:
|
This reverts commit faae27b.
This reverts commit faae27b.
This reverts commit 4d94823.
SUMMARY
Fixes: #14672
The function
postForm
introduced in PR #9345 has the target set to_blank
by default. This PR specifies_self
instead. There does not seem to be any obvious issue in doing so. Input from @graceguo-supercat is appreciated as a double check.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION