[Kedro-viz-890]/run-export-modal #898

Huongg · 2022-06-09T16:32:03Z

Description

Resolves #890: To create a modal to notify users when they download a run in the experiment tracking

Development notes

QA notes

There are tests to ensure both buttons work as expected

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

Signed-off-by: huongg <[email protected]>

Huongg · 2022-06-09T16:36:13Z

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

+import { configure, mount, shallow } from 'enzyme';
+import { render, screen } from '@testing-library/react';
+
+// to help find text which is made by multiple HTML elements


Is there a place I can put this util function? I think we can re-use it somewhere else later

i was thinking maybe in src/components/ui/button we could add a success button?

yeah i was thinking the same, what do you think of naming it button-success. It sounds weird but if we name it this way, all the button components display next to each other.
eg

- button - button-success

what do you think @rashidakanchwala @tynandebold ?

We should probably generalize it more than that. At some point we'll probably need to supply a fail state, too. So maybe we do something like button-message or similar.

Also to your original question about the utility function: you could just export const findTextWithTags for now, because at the moment I don't think we need it anywhere else, so it may be premature to move it elsewhere. If you want to move it though, somewhere in one of the utils files would be best, but you'd probably want to create a new file, like test-utils.js, so maybe we hold off for now.

Huongg · 2022-06-09T16:40:24Z

src/components/experiment-tracking/run-export-modal.js/run-export-modal.js

+  const [exportData, setExportData] = useState([]);
+  const [isExported, setIsExported] = useState(false);
+
+  const updateExportData = useCallback(() => {


I decided to leave
this function here locally in the component itself, rather than in the parent component experiment-primary-toolbar, or experiment-wrapper, as it's easier to manage local state, rather than passing it down through props

tynandebold · 2022-06-10T08:50:39Z

Congratulations on your first PR in Viz!!! 🎆

Just one observation before a full review: I believe the entire button background should turn green on the success state, as it does in this modal:

Signed-off-by: huongg <[email protected]>

…/kedro-viz into kedro-viz-890/run-export-modal Signed-off-by: huongg <[email protected]>

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

Signed-off-by: huongg <[email protected]>

rashidakanchwala

thanks @Huongg -- LGTM :)

tynandebold

Great work here! It's nearly good to go.

I'm requesting changes and writing only one comment for now. This implementation changes the way to modals are animating in/out, and I don't think it should. We'll need to tweak the changes to somehow fix that.

tynandebold · 2022-06-14T10:58:01Z

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

+        {showRunDetailsModal && (
+          <RunDetailsModal
+            runMetadataToEdit={runMetadataToEdit}
+            runs={runMetadata}
+            setShowRunDetailsModal={setShowRunDetailsModal}
+            theme={theme}
+            visible={showRunDetailsModal}
+          />
+        )}
+        {showRunExportModal && (
+          <RunExportModal
+            theme={theme}
+            visible={showRunExportModal}
+            setShowRunExportModal={setShowRunExportModal}
+            runMetadata={runMetadata}
+            runTrackingData={runTrackingData}
+          />
+        )}


I asked about this yesterday when you were getting a sense check on this approach, and now looking at this work locally it seems that this does change the behavior of the modals animating in and out.

If you look on the main branch you'll see that when they enter there's a fade in and up, and when they disappear it's down and out. We'll need to keep that UX here, too, which means we'll have to find a way not to do this showRunExportModal && or do that and still animate.

hey @tynandebold such a good spot, you're right. We should not remove the animation, sorry i didnt realise it yesterday. Actually, i can just use the existing props visible to apply the context, rather than check with props to render a specific component

Let me know what you think about the latest changes? Happy to discuss further

Signed-off-by: huongg <[email protected]>

tynandebold

Excellent changes. The animations are back! Thank you for that. I'm approving now with some comments and a couple of thoughts:

When I toggle through the RunExportModal with my keyboard and export the data the modal animation doesn't fire nor does the modal close. It does work on the RunDetailsModal though.
I like the use of context, I just hope that if we need to react to a failed API request and then show a failed state in the button instead of the success we'll be able to. We will, right?

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

src/components/experiment-tracking/run-export-modal.js/run-export-modal.js

src/components/experiment-tracking/run-export-modal.js/run-export-modal.scss

src/utils/button-timeout-context.js

Signed-off-by: huongg <[email protected]>

Huongg · 2022-06-15T08:39:05Z

Excellent changes. The animations are back! Thank you for that. I'm approving now with some comments and a couple of thoughts:

When I toggle through the RunExportModal with my keyboard and export the data the modal animation doesn't fire nor does the modal close. It does work on the RunDetailsModal though.

I like the use of context, I just hope that if we need to react to a failed API request and then show a failed state in the button instead of the success we'll be able to. We will, right?

hey to answer your question as below

yeah you're right it doesn't have the animation at the moment when using the keyboard. I guess because it was wrapped around by a CSV link. I'll think of a way to fix it but for now let's merge this ticket and I'll open a new bug for that?
yeah definitely, we just need to introduce a new state in the context to handle the failed state

tynandebold · 2022-06-15T10:11:24Z

Excellent changes. The animations are back! Thank you for that. I'm approving now with some comments and a couple of thoughts:

When I toggle through the RunExportModal with my keyboard and export the data the modal animation doesn't fire nor does the modal close. It does work on the RunDetailsModal though.

I like the use of context, I just hope that if we need to react to a failed API request and then show a failed state in the button instead of the success we'll be able to. We will, right?

hey to answer your question as below

yeah you're right it doesn't have the animation at the moment when using the keyboard. I guess because it was wrapped around by a CSV link. I'll think of a way to fix it but for now let's merge this ticket and I'll open a new bug for that?

yeah definitely, we just need to introduce a new state in the context to handle the failed state

Perfect ☺️

Yes, let's create a new ticket to track that keyboard bug. And good to know about the failed state. Thanks for confirming. Merge this whenever you can!

Huongg added 8 commits June 9, 2022 11:45

run-export-modal component

dccf5f2

Signed-off-by: huongg <[email protected]>

pass state to RunExportModal

1163132

Signed-off-by: huongg <[email protected]>

move callback handle export to its own component

d0af3d5

Signed-off-by: huongg <[email protected]>

import RunExportModal in Details component

905caa9

Signed-off-by: huongg <[email protected]>

remove console.log

8ee6f7c

Signed-off-by: huongg <[email protected]>

set fix width for primary button

f71d3aa

Signed-off-by: huongg <[email protected]>

update timing

364d90f

Signed-off-by: huongg <[email protected]>

test for RunExportModal

02fde8d

Signed-off-by: huongg <[email protected]>

Huongg requested review from tynandebold, cvetankanechevska and rashidakanchwala June 9, 2022 16:32

Huongg commented Jun 9, 2022

View reviewed changes

Huongg added 9 commits June 10, 2022 10:21

run-export-modal component

146916e

Signed-off-by: huongg <[email protected]>

pass state to RunExportModal

bbab40a

Signed-off-by: huongg <[email protected]>

move callback handle export to its own component

cb69bd2

Signed-off-by: huongg <[email protected]>

import RunExportModal in Details component

269731d

Signed-off-by: huongg <[email protected]>

remove console.log

c6d733f

Signed-off-by: huongg <[email protected]>

set fix width for primary button

f968c68

Signed-off-by: huongg <[email protected]>

update timing

b12dc20

Signed-off-by: huongg <[email protected]>

test for RunExportModal

78ced62

Signed-off-by: huongg <[email protected]>

Merge branch 'kedro-viz-890/run-export-modal' of github.com:kedro-org…

6339920

…/kedro-viz into kedro-viz-890/run-export-modal Signed-off-by: huongg <[email protected]>

rashidakanchwala reviewed Jun 10, 2022

View reviewed changes

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

rashidakanchwala reviewed Jun 10, 2022

View reviewed changes

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

Huongg added 5 commits June 13, 2022 15:52

update classname and label

b69ab00

Signed-off-by: huongg <[email protected]>

create context for button timeout

c300702

Signed-off-by: huongg <[email protected]>

use ButtonTimeoutContext in RunExportModal

86bca44

Signed-off-by: huongg <[email protected]>

use ButtonTimeoutContext in RunDetailsModal

28e0ea8

Signed-off-by: huongg <[email protected]>

update test with mock test for Context

f4a61d3

Signed-off-by: huongg <[email protected]>

Merge branch 'main'

25f73af

Signed-off-by: huongg <[email protected]>

Huongg requested a review from rashidakanchwala June 13, 2022 16:41

rashidakanchwala approved these changes Jun 13, 2022

View reviewed changes

tynandebold requested changes Jun 14, 2022

View reviewed changes

use visible props instead

a525385

Signed-off-by: huongg <[email protected]>

Huongg requested a review from tynandebold June 14, 2022 12:50

change hasInteracted to hasNotInteracted

e025dd0

Signed-off-by: huongg <[email protected]>

tynandebold approved these changes Jun 14, 2022

View reviewed changes

Huongg added 3 commits June 14, 2022 21:56

update test to reflect with new props

f9a8993

Signed-off-by: huongg <[email protected]>

sorting a-z

4a0d649

Signed-off-by: huongg <[email protected]>

update description for ButtonTimeoutContext

a4caf9e

Signed-off-by: huongg <[email protected]>

Huongg merged commit cf05cb3 into main Jun 15, 2022

Huongg deleted the kedro-viz-890/run-export-modal branch June 15, 2022 10:36

tynandebold mentioned this pull request Jun 16, 2022

Release v4.7.0 #917

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kedro-viz-890]/run-export-modal #898

[Kedro-viz-890]/run-export-modal #898

Huongg commented Jun 9, 2022 •

edited

Loading

Huongg Jun 9, 2022

rashidakanchwala Jun 10, 2022

Huongg Jun 10, 2022

tynandebold Jun 10, 2022

Huongg Jun 9, 2022 •

edited

Loading

tynandebold commented Jun 10, 2022

rashidakanchwala left a comment

tynandebold left a comment

tynandebold Jun 14, 2022

Huongg Jun 14, 2022

tynandebold left a comment

Huongg commented Jun 15, 2022

tynandebold commented Jun 15, 2022

[Kedro-viz-890]/run-export-modal #898

[Kedro-viz-890]/run-export-modal #898

Conversation

Huongg commented Jun 9, 2022 • edited Loading

Description

Development notes

QA notes

Checklist

Huongg Jun 9, 2022

Choose a reason for hiding this comment

rashidakanchwala Jun 10, 2022

Choose a reason for hiding this comment

Huongg Jun 10, 2022

Choose a reason for hiding this comment

tynandebold Jun 10, 2022

Choose a reason for hiding this comment

Huongg Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

tynandebold commented Jun 10, 2022

rashidakanchwala left a comment

Choose a reason for hiding this comment

tynandebold left a comment

Choose a reason for hiding this comment

tynandebold Jun 14, 2022

Choose a reason for hiding this comment

Huongg Jun 14, 2022

Choose a reason for hiding this comment

tynandebold left a comment

Choose a reason for hiding this comment

Huongg commented Jun 15, 2022

tynandebold commented Jun 15, 2022

Huongg commented Jun 9, 2022 •

edited

Loading

Huongg Jun 9, 2022 •

edited

Loading