[KED-2997] Create rename run and edit notes UI behavior #665

tynandebold · 2021-12-07T12:19:04Z

Description

Development notes

To do this, I've created a couple of new components:

Front-end :

A <RunDetailsModal/> component, based on the <SettingsModal/> that we already had, except this new one isn't connected in any way to Redux. It does share a lot of styles with the <SettingsModal/>, as I'm importing that CSS file.
A new <Input/> component that lives within a new components/ui folder. This component actually uses a <textarea/> HTML element under the hood, which gives good flexibility. And further, going forward, I think we can start to move some UI-only components into this folder and create any new UI components here, as needed.
A <Pencil/> icon component.

Backend :
UpdateRunDetailSuccess now returns a Run with the latest changes requested by the user instead of returning a JSONObject as it did previously.

QA notes

Check out the branch and click on the Edit details pencil icon or the run metadata title/notes text to trigger the modal.

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

…dal working

…into feature/rename-run-and-edit-notes-behavior#ked-2997

…mponent

…into feature/rename-run-and-edit-notes-behavior#ked-2997

studioswong

Great stuff, the UI works great!

I've added some futher comments, in particular some suggestions of taking out the need of the selectedMetadataRunId hook and assigning the run metadata directly on click.

Adding in a request change to also remind us not to merge this in until the mutation endpoints are merged in as a completed feature for this user journey.

src/components/experiment-tracking/details/index.js

studioswong · 2021-12-09T18:09:52Z

src/components/ui/input/index.js

+const MIN_HEIGHT = 20;
+
+const Input = ({
+  characterLimit = false,


if we set this to 0 by default then we would not need the seperate check for isLimitSet

Suggested change

characterLimit = false,

characterLimit = 0,

src/components/ui/input/index.js

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

src/components/experiment-tracking/details/index.js

…into feature/rename-run-and-edit-notes-behavior#ked-2997

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

rashidakanchwala · 2021-12-13T14:39:46Z

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

@@ -0,0 +1,54 @@
+import React from 'react';
+import Button from '@quantumblack/kedro-ui/lib/components/button';
+import Modal from '@quantumblack/kedro-ui/lib/components/modal';


hi, the Kedro-UI modal PR is ready to merge. Just need someone to review the handleKeyevents stuff. Can we use that in this if it gets merged before this PR?

Yes, absolutely we can.

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

…into feature/rename-run-and-edit-notes-behavior#ked-2997

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

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

studioswong · 2022-01-04T19:01:57Z

Thanks for the efforts for aligning on the mutation @tynandebold and @rashidakanchwala.

I tested this branch this morning and just now with the new changes that was merged in earlier today, and noticed the following:

The mutation doesn't seem to be working ( see below screenshot.) I am guessing this might be fixed by the latest fix to the BE that @rashidakanchwala might push to this branch?

This is likely a problem on the FE side - Once submitting a mutation request that failed, clicking on the rename module for other runs will still display the error message ( 'Couldn't update run details. ....' )
This is also likely to be a problem on the FE side - On testing this branch earlier without the latest BE changes (i.e when the mutation is still working), I notice that, on having submitted the mutation successfully while having the rename module still on, the run details on the module jumps to the first run on the sidebar. (I would've capture this error in a gif, but am not able to reproduce this given the existing mutation errors. We can test this and revisit this once the mutation problems are all clear.)

studioswong · 2022-01-04T20:59:37Z

2 more comments about the behaviour of the FE:

As per the design here, the default 'Add here' phrase under the notes input field should not be counted as part of the input characters

Current modal:

Design:

Some weird behaviour I observed - On trying to edit the notes and receiving an error, the note editing field persists with the last entry on clicking a new run - see below.

…ror; use placeholder for notes field

…into feature/rename-run-and-edit-notes-behavior#ked-2997

rashidakanchwala

Hi, looks great and works well. Thanks @tynandebold

Would like @limdauto to review the BE work.

Also nit, currently Apply Changes doesn't close modal. It felt more natural to me it would. But there's no requirements of such in the design.

studioswong · 2022-01-06T10:04:24Z

@tynandebold on checking out this branch and running the app I've been getting the following error - looks like it's worth revisiting?

tynandebold · 2022-01-06T10:09:05Z

@tynandebold on checking out this branch and running the app I've been getting the following error - looks like it's worth revisiting?

@rashidakanchwala was getting the same thing yesterday. I think you just need to restart all servers/npm install/clear cache/etc. It should work after that. Not exactly sure what Rashida did but she got it working after seeing the same error.

…into feature/rename-run-and-edit-notes-behavior#ked-2997

studioswong · 2022-01-06T14:44:50Z

yes indeed weirdly an npm install does the trick - thank you.

The mutation is working well now, and I see that the word count and error message issue has been solved - many thanks for that!

One more observation, on having fill in the note editing field and selecting the cancel option, the previous note entry persists on the modal for a different run ( see below gif) - this should be just a simple fix of just clearing the hook / local state.

Sorry to be thick - We are almost there!

studioswong · 2022-01-06T14:47:52Z

.circleci/config.yml

+            - v${CACHE_VERSION}-{{ arch }}-dependencies-{{ checksum "package.json" }}
+            - v${CACHE_VERSION}-{{ arch }}-dependencies-


are these changes relevant to this PR?

Sort of. They're an addendum to what @limdauto did yesterday.

Yea this is needed to separate linux cache from windows cache in CI.

limdauto

LGTM!

studioswong

works well for me now - let's get this in once the CI is all clear 😉

…into feature/rename-run-and-edit-notes-behavior#ked-2997

* add pencil/edit icon; create run-details modal; open/close of that modal working * add basic text area in modal * local creation of an Input component * create ui folder and add generic textarea and input components * use text-decoration for active state * modal gets dynamic data from selected run * run details modal tests * rename textarea to input and use that in modal; remove other input component * test written for input component * updates based on PR review * Rashida's PR reviews; hide edit run button when comparison view is on * fix typos; update general edit run modal behavior * add runDetails mutation * remove searchable text; add client to mutation so tests pass * don't format schema * Update backend schema * Fixed mypy error * fixed mypy2 * fixed pylint error * update to id from runId in graphql response * update run_id to id in response -2 * UpdateRunDetails returns Run * correct response from updateRunDetails mutation; reset mutation on error; use placeholder for notes field * fix run-selection anomoly * fix failing RunDetailsModal test * update mutation response * use the Modal component from the repo, not the QB UI one * fix failing RunDetailsModal test * remove working_directory from circleci * add working_directory to build_38 * use tmp folder instead of repo * revert and use arch * revert by removing working_directory from build_38 and test build again * clear input on trigger; remove arch from circle ci Co-authored-by: Rashida Kanchwala <[email protected]> Signed-off-by: Rashida Kanchwala <[email protected]>

tynandebold added 11 commits December 2, 2021 17:38

add pencil/edit icon; create run-details modal; open/close of that mo…

2494eb0

…dal working

add basic text area in modal

0bcf22c

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

96b0992

…into feature/rename-run-and-edit-notes-behavior#ked-2997

local creation of an Input component

fe10f11

create ui folder and add generic textarea and input components

2e90f22

use text-decoration for active state

01623a5

modal gets dynamic data from selected run

ce09a90

run details modal tests

15a9690

rename textarea to input and use that in modal; remove other input co…

e1c08f3

…mponent

test written for input component

d644efd

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

7707e34

…into feature/rename-run-and-edit-notes-behavior#ked-2997

tynandebold requested review from limdauto, studioswong and rashidakanchwala December 7, 2021 12:19

studioswong suggested changes Dec 9, 2021

View reviewed changes

tynandebold added 2 commits December 10, 2021 08:57

updates based on PR review

a9e64b3

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

b175e38

…into feature/rename-run-and-edit-notes-behavior#ked-2997

tynandebold requested a review from studioswong December 10, 2021 08:57

rashidakanchwala reviewed Dec 13, 2021

View reviewed changes

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

rashidakanchwala reviewed Dec 13, 2021

View reviewed changes

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

tynandebold added 6 commits December 14, 2021 10:00

Rashida's PR reviews; hide edit run button when comparison view is on

f3a45e0

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

1e7f703

…into feature/rename-run-and-edit-notes-behavior#ked-2997

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

810b2a0

…into feature/rename-run-and-edit-notes-behavior#ked-2997

fix typos; update general edit run modal behavior

9b5fbc9

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

1681b4f

…into feature/rename-run-and-edit-notes-behavior#ked-2997

add runDetails mutation

29919f9

tynandebold requested a review from rashidakanchwala December 20, 2021 17:26

studioswong reviewed Jan 4, 2022

View reviewed changes

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

studioswong reviewed Jan 4, 2022

View reviewed changes

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

update run_id to id in response -2

f595229

rashidakanchwala and others added 8 commits January 4, 2022 21:36

UpdateRunDetails returns Run

63a1320

correct response from updateRunDetails mutation; reset mutation on er…

84173ac

…ror; use placeholder for notes field

fix run-selection anomoly

c35efe5

fix failing RunDetailsModal test

1abf75a

update mutation response

eba39f5

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

471d03d

…into feature/rename-run-and-edit-notes-behavior#ked-2997

use the Modal component from the repo, not the QB UI one

327a71b

fix failing RunDetailsModal test

dcefcca

rashidakanchwala approved these changes Jan 5, 2022

View reviewed changes

tynandebold added 6 commits January 6, 2022 10:46

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

88f9732

…into feature/rename-run-and-edit-notes-behavior#ked-2997

remove working_directory from circleci

e6fd526

add working_directory to build_38

f04307e

use tmp folder instead of repo

3466a2d

revert and use arch

ca51744

revert by removing working_directory from build_38 and test build again

27f13cf

studioswong reviewed Jan 6, 2022

View reviewed changes

limdauto approved these changes Jan 6, 2022

View reviewed changes

clear input on trigger; remove arch from circle ci

b14fb2f

studioswong approved these changes Jan 6, 2022

View reviewed changes

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

18db979

…into feature/rename-run-and-edit-notes-behavior#ked-2997

tynandebold merged commit 81c3bd6 into main Jan 6, 2022

tynandebold deleted the feature/rename-run-and-edit-notes-behavior#ked-2997 branch January 6, 2022 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KED-2997] Create rename run and edit notes UI behavior #665

[KED-2997] Create rename run and edit notes UI behavior #665

tynandebold commented Dec 7, 2021 •

edited by rashidakanchwala

Loading

studioswong left a comment

studioswong Dec 9, 2021

rashidakanchwala Dec 13, 2021

tynandebold Dec 14, 2021

studioswong commented Jan 4, 2022

studioswong commented Jan 4, 2022 •

edited

Loading

rashidakanchwala left a comment

studioswong commented Jan 6, 2022

tynandebold commented Jan 6, 2022

studioswong commented Jan 6, 2022 •

edited

Loading

studioswong Jan 6, 2022

tynandebold Jan 6, 2022

limdauto Jan 6, 2022

limdauto left a comment

studioswong left a comment

		- v${CACHE_VERSION}-{{ arch }}-dependencies-{{ checksum "package.json" }}
		- v${CACHE_VERSION}-{{ arch }}-dependencies-

[KED-2997] Create rename run and edit notes UI behavior #665

[KED-2997] Create rename run and edit notes UI behavior #665

Conversation

tynandebold commented Dec 7, 2021 • edited by rashidakanchwala Loading

Description

Development notes

QA notes

Checklist

studioswong left a comment

Choose a reason for hiding this comment

studioswong Dec 9, 2021

Choose a reason for hiding this comment

rashidakanchwala Dec 13, 2021

Choose a reason for hiding this comment

tynandebold Dec 14, 2021

Choose a reason for hiding this comment

studioswong commented Jan 4, 2022

studioswong commented Jan 4, 2022 • edited Loading

rashidakanchwala left a comment

Choose a reason for hiding this comment

studioswong commented Jan 6, 2022

tynandebold commented Jan 6, 2022

studioswong commented Jan 6, 2022 • edited Loading

studioswong Jan 6, 2022

Choose a reason for hiding this comment

tynandebold Jan 6, 2022

Choose a reason for hiding this comment

limdauto Jan 6, 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 Dec 7, 2021 •

edited by rashidakanchwala

Loading

studioswong commented Jan 4, 2022 •

edited

Loading

studioswong commented Jan 6, 2022 •

edited

Loading