[KED-2996] Feature/toggle bookmark behavior #689

tynandebold · 2022-01-10T13:54:20Z

Description

Resolves KED-2996.

Development notes

This PR enables all the bookmark functionality. There are three places where you can bookmark a run from:

The runs list card
The pipeline toolbar (when not in comparison view)
From the kebab menu (when in comparison view)

Each area is wired up to now be able to toggle bookmarks.

I also wired up the close icon next to the kebab menu. That was a quick win and easy to do.

QA notes

I chose not to include the bookmark help text on hover in the runs list. Here's a design screenshot:

I did this for two reasons:

By default, the help text gets cut off and can't extend outside of the sidebar. There's a lot of overhead here that would have made this seemingly simple change time consuming and, in my opinion, not worthwhile for our MVP approach.
When I did have it there, though not working perfectly, it felt very intrusive with the tooltip coming up over and over and over again.

Separately, I also ordered the icons like this:

And not like this:

Isn't the close icon always on an end? I didn't feel right when it was in the middle.

Checklist

Read the contributing guidelines
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added new entries to the RELEASE.md file
Added tests to cover my changes

…ature/toggle-bookmark-behavior#ked-2996

src/components/experiment-tracking/runs-list/runs-list.scss

rashidakanchwala

LGTM! Thanks Tynan!!!

…ature/toggle-bookmark-behavior#ked-2996

src/components/experiment-tracking/experiment-primary-toolbar/experiment-primary-toolbar.js

src/components/experiment-tracking/run-metadata/run-metadata.js

studioswong · 2022-01-12T10:10:17Z

src/components/experiment-tracking/run-metadata/run-metadata.test.js

+  // Remove for now.
+  // it('enables the pin button when show changes is enabled ', () => {
+  //   const wrapper = mount(
+  //     <RunMetadata
+  //       enableShowChanges={true}
+  //       isSingleRun={false}
+  //       runs={twoRuns}
+  //     />
+  //   );
+
+  //   expect(wrapper.find('.pipeline-menu-button__pin').length).toEqual(4);
+  // });


This test can be put back in once the useMutation hook is moved up to the wrapper level.

On another note, please add relevant tests to cover the introduction of the new kebab menu and the bookmark icon.

I wish this was true. The test still fails even if the mutation hook is moved. The hook itself still fires in this file, so it doesn't make a difference.

I won't be able to write any tests either. As soon as the details-metadata__buttons markup becomes available with two runs in comparison view, we get that Apollo client error I wrote about in the PR description.

I think this problem with useMutation would be something we need to resolve within this PR as this seems to restrict our ability to write tests, which I highly doubt it would be designed this way given how many users there are within the react community.

Throwing a thought here - looking at the error it looks like the test was not able to locate the client, which you might need to wrap the wrapper with an ApolloProvider in the test?

Also, it also does seem weird that this problem surfaces in this PR and not in the previous one when we introduced the mutation hook for the first time - I wonder what is the reason behind that?

it might either suggest that there might be a different setup, or perhaps there wasn't the right tests set up to properly test that 😆

Haha, agreed! I'm looking into this now: https://www.apollographql.com/docs/react/development-testing/testing/

Will report back.

studioswong · 2022-01-12T10:12:14Z

src/components/experiment-tracking/runs-list-card/runs-list-card.js

  const humanReadableTime = toHumanReadableTime(timestamp);

-  const onClick = (id) => {
+  const onClick = (id, e) => {


please rename this to something more description such as onToggleBookmarkButton, etc.

Please also add a test to cover the dead zone scenerio too - it can be just set up to test that updateRunDetailsfunction gets called

This function can either toggle the bookmark or fire the onRunSelection function, so it isn't strictly about the bookmark.

I can change it to onRunsListCardClick. Is that ok?

I added a test but it also fails because of the Apollo bug. Uncomment the code and then try it here:

npm run test src/components/experiment-tracking/runs-list-card/runs-list-card.test.js

src/components/experiment-tracking/runs-list-card/runs-list-card.js

studioswong

Thanks for setting this up, the bookmark journey works well in general.

One thing I noticed. - on toggling 'show changes' off the entire burger menu, including the toggle bookmark buttons, etc, also gets toggled off, while instead they should still exist and only the 'pin run' button should be toggled off.

I'll continue to go through some of the code and leave further single comments if I see anything.

…ature/toggle-bookmark-behavior#ked-2996

tynandebold · 2022-01-12T12:40:15Z

Thanks for setting this up, the bookmark journey works well in general.

One thing I noticed. - on toggling 'show changes' off the entire burger menu, including the toggle bookmark buttons, etc, also gets toggled off, while instead they should still exist and only the 'pin run' button should be toggled off.

I'll continue to go through some of the code and leave further single comments if I see anything.

Updated everything you asked for. Lmk if there's anything else :)

…ithub.com/kedro-org/kedro-viz into feature/toggle-bookmark-behavior#ked-2996

…panel

…ature/toggle-bookmark-behavior#ked-2996

studioswong · 2022-01-18T13:54:09Z

src/apollo/mutations.js

@@ -22,10 +21,8 @@ export const UPDATE_RUN_DETAILS = gql`
 `;

 export const useUpdateRunDetails = () => {
-  const [_updateRunDetails, { error, loading, reset }] = useMutation(
-    UPDATE_RUN_DETAILS,
-    { client }


Is this the object the one that was causing the problems for the test?

No it wasn't that. It's just we were't actually mocking the useUpdateRunDetails mutation.

indeed. I've always wondered why we need to pass in the client here as it's passed on from the ApolloProvider anyways, good to delete this here.

limdauto

I haven't tested the functionality but the code LGTM!

src/apollo/queries.js

src/components/experiment-tracking/run-details-modal/run-details-modal.test.js

RELEASE.md

src/components/experiment-wrapper/experiment-wrapper.js

studioswong

LGTM, thanks for getting through with the fix to the tests!

I've left a few comments which I'll let you fix them before merging it in. For the tests for run-details-modal, I'll let you decide on whether to tackle it in this PR or as a separate PR.

tynandebold added 6 commits January 7, 2022 15:26

toggling working on the primary toolbar

ae3c0c3

toggling bookmark working on run list cards

9da2be9

scrolling runs list; sticky comparison toggle

277b098

update comment

d2add95

kebab menu and bookmark nearly working

a9393aa

bookmark working via kebab menu

353779e

tynandebold requested review from limdauto, studioswong and rashidakanchwala January 10, 2022 13:54

tynandebold added 2 commits January 10, 2022 13:54

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…

e2e4daa

…ature/toggle-bookmark-behavior#ked-2996

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…

7073f1d

…ature/toggle-bookmark-behavior#ked-2996

rashidakanchwala reviewed Jan 11, 2022

View reviewed changes

src/components/experiment-tracking/runs-list/runs-list.scss Outdated Show resolved Hide resolved

rashidakanchwala approved these changes Jan 11, 2022

View reviewed changes

tynandebold added 2 commits January 11, 2022 14:44

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…

73b3c88

…ature/toggle-bookmark-behavior#ked-2996

remove commented out code and unused styles

c6ddf2e

studioswong reviewed Jan 12, 2022

View reviewed changes

src/components/experiment-tracking/experiment-primary-toolbar/experiment-primary-toolbar.js Outdated Show resolved Hide resolved

studioswong reviewed Jan 12, 2022

View reviewed changes

src/components/experiment-tracking/run-metadata/run-metadata.js Outdated Show resolved Hide resolved

studioswong reviewed Jan 12, 2022

View reviewed changes

src/components/experiment-tracking/runs-list-card/runs-list-card.js Outdated Show resolved Hide resolved

studioswong suggested changes Jan 12, 2022

View reviewed changes

tynandebold added 3 commits January 12, 2022 10:30

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…

e4e727c

…ature/toggle-bookmark-behavior#ked-2996

extract updateRunDetails mutation to it's own hook

316299d

add tests; other PR fixes

c19d8b5

tynandebold requested a review from studioswong January 12, 2022 12:39

studioswong and others added 4 commits January 17, 2022 14:32

change test from mount to shallow render

1ae1ae2

Merge branch 'feature/toggle-bookmark-behavior#ked-2996' of https://g…

867bd18

…ithub.com/kedro-org/kedro-viz into feature/toggle-bookmark-behavior#ked-2996

update overflowing run title text to mirror file path in metdatadata …

9c4be42

…panel

update test

bb7ab09

tynandebold added 4 commits January 17, 2022 17:34

update test

1332424

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…

2d09f4a

…ature/toggle-bookmark-behavior#ked-2996

fix failing tests

e025510

add note to release file

348a4aa

tynandebold requested a review from yetudada as a code owner January 18, 2022 13:51

studioswong reviewed Jan 18, 2022

View reviewed changes

tynandebold requested a review from studioswong January 18, 2022 13:55

limdauto approved these changes Jan 18, 2022

View reviewed changes

src/apollo/queries.js Show resolved Hide resolved

remove duplicated id field in getRunMetadata query

4a420b9

studioswong reviewed Jan 18, 2022

View reviewed changes

src/components/experiment-tracking/run-details-modal/run-details-modal.test.js Show resolved Hide resolved

studioswong reviewed Jan 18, 2022

View reviewed changes

RELEASE.md Outdated Show resolved Hide resolved

studioswong reviewed Jan 18, 2022

View reviewed changes

src/components/experiment-wrapper/experiment-wrapper.js Outdated Show resolved Hide resolved

studioswong approved these changes Jan 18, 2022

View reviewed changes

tynandebold added 3 commits January 18, 2022 16:53

add test

c959d15

remove import

ccc2533

update comment

7230295

tynandebold merged commit 2c19edc into main Jan 18, 2022

tynandebold deleted the feature/toggle-bookmark-behavior#ked-2996 branch January 18, 2022 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KED-2996] Feature/toggle bookmark behavior #689

[KED-2996] Feature/toggle bookmark behavior #689

tynandebold commented Jan 10, 2022 •

edited

Loading

rashidakanchwala left a comment

studioswong Jan 12, 2022

tynandebold Jan 12, 2022 •

edited

Loading

studioswong Jan 13, 2022

studioswong Jan 13, 2022

tynandebold Jan 13, 2022

studioswong Jan 12, 2022 •

edited

Loading

tynandebold Jan 12, 2022 •

edited

Loading

studioswong left a comment

tynandebold commented Jan 12, 2022

studioswong Jan 18, 2022

tynandebold Jan 18, 2022

studioswong Jan 18, 2022

limdauto left a comment

studioswong left a comment

[KED-2996] Feature/toggle bookmark behavior #689

[KED-2996] Feature/toggle bookmark behavior #689

Conversation

tynandebold commented Jan 10, 2022 • edited Loading

Description

Development notes

QA notes

Checklist

rashidakanchwala left a comment

Choose a reason for hiding this comment

studioswong Jan 12, 2022

Choose a reason for hiding this comment

tynandebold Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

studioswong Jan 13, 2022

Choose a reason for hiding this comment

studioswong Jan 13, 2022

Choose a reason for hiding this comment

tynandebold Jan 13, 2022

Choose a reason for hiding this comment

studioswong Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

tynandebold Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

studioswong left a comment

Choose a reason for hiding this comment

tynandebold commented Jan 12, 2022

studioswong Jan 18, 2022

Choose a reason for hiding this comment

tynandebold Jan 18, 2022

Choose a reason for hiding this comment

studioswong Jan 18, 2022

Choose a reason for hiding this comment

limdauto left a comment

Choose a reason for hiding this comment

studioswong left a comment

Choose a reason for hiding this comment

tynandebold commented Jan 10, 2022 •

edited

Loading

tynandebold Jan 12, 2022 •

edited

Loading

studioswong Jan 12, 2022 •

edited

Loading

tynandebold Jan 12, 2022 •

edited

Loading