feat(dashboard): direct link to single chart/tab/header in dashboard #6964

graceguo-supercat · 2019-03-01T17:21:10Z

We always render first tab when dashboard opens. When a dashboard has multiple root tabs and nested row-level tabs, it will takes a few clicks to navigate to the point of interest. This feature is to add direct link for a UI component (chart, tab, header) in dashboard.

this feature depends on a update on dashboard layout metadata[[db migration] add parent ids into Dashboard layout metadata #6945]. Every UI component keeps a list of parents component ids that can traverse from root element.
We can use anchor link (hash) in the url to indicate which component to show at page landing time.
At dashboard initial time, if url has anchor link hash, dashboard can find the directLinkPath which is the list of component ids from root to element id.
When TABS component is mounted, if it has directLinkPath prop, it will choose the child in the directLinkPath to display.
After user finish edit dashboard and right before save to database, we traverse dashboard layout tree from root component, and update parents attribute for each UI component.

Result

Direct link to tab

Direct link to chart

Direct link to header

@williaster @kristw @michellethomas @elibrumbaugh @mistercrunch

elibrumbaugh · 2019-03-05T01:17:22Z

Grace and I synced up offline about this today. Documenting the outcome here.

Move the link icon from the left of the tab to the right. This will allow the user to read the title before link action.
Remove the link icon from chart titles and move it into the chart overflow menu with the other chart related actions. The action will be title share chart This has better parody with the dashboard share interaction.

TBD: Size of the icon. Is it large enough? Most likely wait for user feedback on this. Right now it is the same size as the icon in the chart share action.

williaster · 2019-03-05T07:08:08Z

design plan sounds great!

codecov-io · 2019-03-11T17:16:38Z

Codecov Report

Merging #6964 into master will increase coverage by 0.09%.
The diff coverage is 78.16%.

@@            Coverage Diff             @@
##           master    #6964      +/-   ##
==========================================
+ Coverage   64.57%   64.67%   +0.09%     
==========================================
  Files         422      425       +3     
  Lines       20581    20660      +79     
  Branches     2251     2280      +29     
==========================================
+ Hits        13290    13361      +71     
- Misses       7169     7175       +6     
- Partials      122      124       +2

Impacted Files	Coverage Δ
...perset/assets/src/dashboard/util/getEmptyLayout.js	`0% <ø> (ø)`	⬆️
...erset/assets/src/components/URLShortLinkButton.jsx	`76.47% <ø> (ø)`	⬆️
...et/assets/src/dashboard/components/SliceHeader.jsx	`24% <ø> (ø)`	⬆️
...sets/src/dashboard/containers/DashboardBuilder.jsx	`0% <ø> (ø)`	⬆️
superset/assets/src/dashboard/containers/Chart.jsx	`90.9% <ø> (ø)`	⬆️
.../src/dashboard/components/gridComponents/Chart.jsx	`69.73% <ø> (ø)`	⬆️
...t/assets/src/dashboard/reducers/getInitialState.js	`0% <0%> (ø)`	⬆️
...t/assets/src/dashboard/reducers/dashboardLayout.js	`94.84% <0%> (-4.08%)`	⬇️
...s/src/dashboard/components/SliceHeaderControls.jsx	`11.62% <0%> (ø)`	⬆️
...src/dashboard/components/HeaderActionsDropdown.jsx	`67.44% <100%> (ø)`	⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60a7b6d...10c97fa. Read the comment docs.

williaster

Looking great overall! I had a few suggestions.

I would vote to change the name directLinkPath to something that more clearly indicates what it is if you don't have context of this mechanism, "directLinkPath" could be anything. I think something like nestedLinkPath or parentIds or something could be more readable which I think is important, wdyt?

williaster · 2019-03-11T17:59:30Z

superset/assets/spec/javascripts/components/AnchorLink_spec.jsx

+    anchorLinkId: 'CHART-123',
+  };
+
+  it('ComponentDidMount', () => {


it's not clear from this test name what's supposed to happen in componentDidMount

my intention is this:
if location hash matched with component id, when AnchorLink is mounted, it should call DOM function scrollIntoView (bring chart/tab/header into center of view).
here is a discuss about similar way to test: enzymejs/enzyme#676

sorry I mean include a description of what you expect in the it(_test description_, () => ...), componentDidMount is not a description of what is expected to happen in componentDidMount, if I read "componentDidMount" in the CLI or CI report, I would have not clue what went wrong or what was expected to happen that failed.

e.g., something like it('scrolls the AnchorLink into view upon mount', ...) is all I meant.

😂 description is updated.

williaster · 2019-03-11T18:05:01Z

superset/assets/src/components/AnchorLink.jsx

+  anchorLinkId: PropTypes.string.isRequired,
+  filters: PropTypes.object,
+  showShortLinkButton: PropTypes.bool,
+  placement: PropTypes.string,


could you enumerate the allowed types instead of simply string? more specific = better.

williaster · 2019-03-11T18:06:39Z

superset/assets/src/dashboard/containers/DashboardComponent.jsx

@@ -43,6 +43,11 @@ const propTypes = {
  updateComponents: PropTypes.func.isRequired,
  handleComponentDrop: PropTypes.func.isRequired,
  logEvent: PropTypes.func.isRequired,
+  directLinkPath: PropTypes.array,


array of what?

williaster · 2019-03-11T18:07:41Z

superset/assets/src/dashboard/util/findTabIndexByComponentId.js

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+export default function({ currentComponent, directLinkPath = [] }) {


you should always name functions, it enables a better debugging experience.

williaster · 2019-03-11T18:12:27Z

superset/assets/src/components/URLShortLinkButton.jsx

@@ -29,6 +29,7 @@ const propTypes = {
  emailSubject: PropTypes.string,
  emailContent: PropTypes.string,
  addDangerToast: PropTypes.func.isRequired,
+  placement: PropTypes.string,


can enumerate specific values here

williaster · 2019-03-11T18:14:16Z

superset/assets/src/dashboard/actions/dashboardState.js

-  return dispatch =>
-    SupersetClient.post({
+  return dispatch => {
+    dispatch({ type: UPDATE_COMPONENTS_PARENTS_LIST });


if this is a Promise you should be able to chain it with the POST request. Is there a reason not to do that?

dispatch is synchronous function, the dispatched action can be asynchronous, like returning a promise, so that we can chain actions after it.
https://stackoverflow.com/questions/43276291/is-store-dispatch-in-redux-synchronous-or-asynchronous

The action { type: UPDATE_COMPONENTS_PARENTS_LIST } is synchronous. Browser will always run this action first, then do post. (so that no need to chain in Promise object)

williaster · 2019-03-11T18:15:34Z

superset/assets/src/dashboard/actions/dashboardState.js

+      .then(response => {
+        dispatch(saveDashboardRequestSuccess());
+        dispatch(addSuccessToast(t('This dashboard was saved successfully.')));
+        return response;


typically the assumption with redux actions is that you can continue to chain promises onto them. are you certain that returning a response doesn't break this promise chain? that's why it was written the way it was.

Yes, tested dashboard save flow. For the same reason (both these 2 actions are synchronous), i just feel no need to wrap them into Promise.

williaster · 2019-03-11T18:15:41Z

superset/assets/src/dashboard/components/DashboardBuilder.jsx

@@ -54,10 +55,12 @@ const propTypes = {
  showBuilderPane: PropTypes.bool,
  handleComponentDrop: PropTypes.func.isRequired,
  toggleBuilderPane: PropTypes.func.isRequired,
+  directLinkPath: PropTypes.array,


array of what?

williaster · 2019-03-11T18:17:16Z

superset/assets/src/dashboard/components/gridComponents/Tabs.jsx

@@ -45,6 +46,7 @@ const propTypes = {
  editMode: PropTypes.bool.isRequired,
  renderHoverMenu: PropTypes.bool,
  logEvent: PropTypes.func.isRequired,
+  directLinkPath: PropTypes.array,


array of what?

williaster · 2019-03-11T18:18:31Z

superset/assets/src/dashboard/reducers/dashboardLayout.js

+      ...state,
+    };
+
+    const doUpdate = currentComponent => {


can you please aadd a component for what this is doing? since it's somewhat complex and very important!

I separated this function out into a util function, so that easier to add test.

graceguo-supercat · 2019-03-11T19:25:56Z

I am thinking to name prop as directPathToChild. Its value is a list of component id ( array of string). As to dashboard initial state, it indicates the path from root component to a specific child.

graceguo-supercat · 2019-03-18T17:42:55Z

ping @williaster all comments are address.

williaster · 2019-03-18T20:04:58Z

superset/assets/src/dashboard/util/findTabIndexByComponentId.js

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+export default function findTabIndexByComponentId({


not sure if we should have unit tests for this or not?

added a couple tests.

williaster

a couple small comments but looks good overall 👍 can you post a final screenshot of the updated UI/designs?

graceguo-supercat · 2019-03-20T21:44:31Z

the expected UI is updated in description Result. I didn't merge this PR yet. so if you have any comments about anchor link UI like size/color/position please let me know. thanks! also ping @elibrumbaugh

williaster · 2019-03-21T17:42:31Z

@elibrumbaugh do you want to do any more design review? overall looks good to me except the link icons don't look vertically aligned with the tab/title text. I'd expect them to be vertically centered instead of the link top-aligned with the text.

graceguo-supercat · 2019-03-25T01:25:29Z

center align anchor link:

graceguo-supercat changed the title ~~[dashboard] direct link to UI component in dashboard~~ [WIP][dashboard] direct link to UI component in dashboard Mar 1, 2019

graceguo-supercat force-pushed the gg-directLink branch 2 times, most recently from d27f199 to 568120a Compare March 1, 2019 23:18

direct display for pre-selected tab

5ae9df0

graceguo-supercat force-pushed the gg-directLink branch from 568120a to 7ca14b5 Compare March 5, 2019 08:24

graceguo-supercat mentioned this pull request Mar 5, 2019

[db migration] add parent ids into Dashboard layout metadata #6945

Merged

update parents

ac7def8

graceguo-supercat force-pushed the gg-directLink branch from 7ca14b5 to b0c5d5c Compare March 11, 2019 15:54

graceguo-supercat changed the title ~~[WIP][dashboard] direct link to UI component in dashboard~~ [dashboard] direct link to UI component in dashboard Mar 11, 2019

graceguo-supercat added the review label Mar 11, 2019

graceguo-supercat changed the title ~~[dashboard] direct link to UI component in dashboard~~ [dashboard] direct link to single chart/tab/header in dashboard Mar 11, 2019

graceguo-supercat added the enhancement:request Enhancement request submitted by anyone from the community label Mar 11, 2019

williaster reviewed Mar 11, 2019

View reviewed changes

graceguo-supercat force-pushed the gg-directLink branch from b0c5d5c to 3267255 Compare March 14, 2019 07:41

williaster reviewed Mar 18, 2019

View reviewed changes

williaster approved these changes Mar 18, 2019

View reviewed changes

kristw changed the title ~~[dashboard] direct link to single chart/tab/header in dashboard~~ feat(dashboard): direct link to single chart/tab/header in dashboard Mar 19, 2019

graceguo-supercat force-pushed the gg-directLink branch from 3267255 to 5698b58 Compare March 20, 2019 21:30

Grace Guo added 2 commits March 24, 2019 18:23

add AnchorLink component

cdda394

add unit tests

10c97fa

graceguo-supercat force-pushed the gg-directLink branch from 5698b58 to 10c97fa Compare March 25, 2019 01:24

kristw mentioned this pull request Mar 27, 2019

[Dashboard] Landing on specific tabs from URL #7116

Closed

graceguo-supercat merged commit c50e6bc into apache:master Apr 9, 2019

graceguo-supercat deleted the gg-directLink branch June 11, 2020 23:21

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 2024

feat(dashboard): direct link to single chart/tab/header in dashboard #6964

feat(dashboard): direct link to single chart/tab/header in dashboard #6964

Conversation

graceguo-supercat commented Mar 1, 2019 • edited Loading

elibrumbaugh commented Mar 5, 2019

williaster commented Mar 5, 2019

codecov-io commented Mar 11, 2019 • edited Loading

Codecov Report

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Mar 11, 2019

graceguo-supercat commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster left a comment

Choose a reason for hiding this comment

graceguo-supercat commented Mar 20, 2019 • edited Loading

williaster commented Mar 21, 2019

graceguo-supercat commented Mar 25, 2019

graceguo-supercat commented Mar 1, 2019 •

edited

Loading

codecov-io commented Mar 11, 2019 •

edited

Loading

graceguo-supercat Mar 14, 2019 •

edited

Loading

graceguo-supercat commented Mar 20, 2019 •

edited

Loading