[KED-2982] Re-write Kedro Dropdown and Menu-option #721

rashidakanchwala · 2022-01-27T15:14:32Z

Description

This PR is a part of the Kedro-UI migration ticket. In this PR we migrate the dropdown and menu-options functionality from Kedro-ui to Kedro-viz

Development notes

QA notes

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: Rashida Kanchwala <[email protected]>

studioswong

Thanks for setting this up.

On testing the new dropdown the 'expand more' arrow is current missing - please see below

Dropdown in this PR:

Dropdown in current demo:

Part of the main goal of this migration work is to refactor the components and bringing the code up to date with the current react setup in our codebase, which includes stripping away the react lifecycle and local state setup to use react hooks. As per my comments below, please refactor the component with the new react hooks setup.

Thank you!

studioswong · 2022-01-31T12:43:11Z

src/components/dropdown/dropdown.js

+  /**
+   * React lifecycle method
+   * {@link https://facebook.github.io/react/docs/react-component.html#componentwillupdate}
+   * @param {Object} New component props
+   */
+  componentDidUpdate(prevProps) {
+    if (this._childrenHaveChanged(prevProps)) {
+      this.setState({
+        selectedOption: this._findSelectedOption(prevProps),
+      });
+    }
+  }
+
+  /**
+   * React lifecycle method
+   * {@link https://facebook.github.io/react/docs/react-component.html#componentwillunmount}
+   * @return {object} JSX for this component
+   */
+  componentWillUnmount() {
+    EventController.removeBodyListeners();
+  }


refer to above - this needs refactoring

studioswong · 2022-01-31T12:44:41Z

src/components/dropdown/dropdown.js

+   * @param {object} e - event object
+   */
+  _handleBodyClicked(e) {
+    if (!this.dropdown.contains(e.target) && this.state.open) {


similar to above, there won't be any this.state within react hooks setup - all references to this.state would now refer to a react hook setState function.

studioswong · 2022-01-31T12:45:04Z

src/components/dropdown/dropdown.js

+      EventController.addBodyListener(this._handleBodyClicked);
+    }
+
+    this.setState({ open: !open }, callback);


same as above - this needs to be set up as a react hook.

studioswong · 2022-01-31T12:46:12Z

src/components/dropdown/dropdown.js

+      this.setState({ open: false, selectedOption }, () => {
+        if (typeof onChanged === 'function') {
+          onChanged(obj);
+        }
+
+        if (typeof onClosed === 'function') {
+          onClosed();
+        }
+      });
+    } else {
+      this.setState({ open: false }, () => {
+        if (typeof onClosed === 'function') {
+          onClosed();
+        }


These would need to be refactored as a hook for all the setState events here - this applies to all other instances of setState in this file as well

studioswong · 2022-01-31T12:48:38Z

src/components/dropdown/dropdown.js

+   */
+  render() {
+    const { children, defaultText, disabled, width } = this.props;
+    const { open, focusedOption, selectedOption } = this.state;


all these state events should be set up as individual hooks

studioswong · 2022-01-31T12:52:07Z

src/components/dropdown/dropdown.js

+  constructor(props) {
+    super(props);
+
+    this.displayName = 'Dropdown';
+
+    // bind method scope
+    this._handleRef = this._handleRef.bind(this);
+    this._getOptionsList = this._getOptionsList.bind(this);
+    this._handleLabelClicked = this._handleLabelClicked.bind(this);
+    this._handleOptionSelected = this._handleOptionSelected.bind(this);
+    this._handleFocusChange = this._handleFocusChange.bind(this);
+    this._handleBodyClicked = this._handleBodyClicked.bind(this);
+    this.open = this.open.bind(this);
+    this.close = this.close.bind(this);
+
+    this.state = {
+      focusedOption: null,
+      selectedOption: this._findSelectedOption(),
+      open: false,
+    };
+  }


Given that we are using react hooks within our codebase, we won't be using the react lifecycles. Please rewrite refactor this using the react hook setup.

studioswong · 2022-01-31T12:53:37Z

src/components/dropdown/dropdown.js

+  /**
+   * React lifecycle method
+   * {@link https://facebook.github.io/react/docs/react-component.html#componentwillupdate}
+   * @param {Object} New component props
+   */
+  componentDidUpdate(prevProps) {
+    if (this._childrenHaveChanged(prevProps)) {
+      this.setState({
+        selectedOption: this._findSelectedOption(prevProps),
+      });
+    }
+  }


same as above - this need to be refactored into a react hook (using useEffect hook)

Thanks, I have moved this PR to draft stage now, and I will work on changing to hooks before releasing it for review again.

tynandebold · 2022-02-08T15:24:09Z

@studioswong after looking at this for well over an hour just now, I'm of the opinion we shouldn't convert this to use hooks at this time. The amount of effort Rashida and I have put in to this is much too great a cost for what we gain, which really isn't anything other than a stylistic alignment of the codebase to use one React pattern. I'm confident we'll be able to live with two components still using the class style, the App component being the other one.

Please let us know what you think. Again, I strongly feel we should leave it as a class given the amount of work involved. We'll then be able to finish the migration epic, and maybe even the sprint. We can revisit this and the App conversion at a later date, as one ticket.

cc @yetudada @idanov

studioswong · 2022-02-08T15:38:34Z

Thanks for looking into this @tynandebold - may I ask what is the main problem you and Rashida encountered in the refactoring to use hooks?

I understand this for now may seem to be a matter of code style, however this has implications down the line as we maintain this piece of work later on - do we still continue down the line of deviating this component from hooks? Do we need to at some point just bite the bullet to refactor this? Don't want to be a blocker, but if we are doing the migration we might as well invest a bit more time to do it now while we are at it. ( broken window analogy, if you will.)

Regardless, I will check out this branch and take a look 😃

cc @yetudada @idanov

rashidakanchwala · 2022-02-08T15:42:24Z

Thanks for looking into this @tynandebold - may I ask what is the main problem you and Rashida encountered in the refactoring to use hooks?

I understand this for now may seem to be a matter of code style, however this has implications down the line as we maintain this piece of work later on - do we still continue down the line of deviating this component from hooks? Do we need to at some point just bite the bullet to refactor this? Don't want to be a blocker, but if we are doing the migration we might as well invest a bit more time to do it now while we are at it. ( broken window analogy, if you will.)

Regardless, I will check out this branch and take a look 😃

cc @yetudada @idanov

Thanks @studioswong appreciate if you can take over and have a look and see if you can fix it!!

tynandebold · 2022-02-08T16:41:02Z

Thanks for looking into this @tynandebold - may I ask what is the main problem you and Rashida encountered in the refactoring to use hooks?

I understand this for now may seem to be a matter of code style, however this has implications down the line as we maintain this piece of work later on - do we still continue down the line of deviating this component from hooks? Do we need to at some point just bite the bullet to refactor this? Don't want to be a blocker, but if we are doing the migration we might as well invest a bit more time to do it now while we are at it. ( broken window analogy, if you will.)

Regardless, I will check out this branch and take a look 😃

cc @yetudada @idanov

The main problem is time invested versus payoff. I think there are other things we can do with our time that will benefit the product and thus our users. I also don't remember both the migration of these components to Viz and also updating the pattern of how they are written as part of the epic. I thought it was just the former, though I do agree it should be aligned.

And to answer your questions:

do we still continue down the line of deviating this component from hooks?

No, we combine this rewrite and the one for the App component together in one ticket since it's now taking the time of three developers to do.

Do we need to at some point just bite the bullet to refactor this?

Yes, as stated above.

studioswong · 2022-02-09T11:44:58Z

The main problem is time invested versus payoff. I think there are other things we can do with our time that will benefit the product and thus our users. I also don't remember both the migration of these components to Viz and also updating the pattern of how they are written as part of the epic. I thought it was just the former, though I do agree it should be aligned.

If we recall from the beginning, I actually don't think the idea of doing this migration is good given the exact problems we face now ( the need to migrate / refactor and maintain legacy react code in our codebase). But I understand there are benefits down the line ( such as being able to have full control and modify this component without the need to publish new versions of the kedro-ui npm package), and it is exactly for this reason to be able to maintain and make changes to this code that we have to do the rewrite. I would pick clearing tech debt and helping us run faster in the future over any inconvenience now - We are just biting the bullet for bigger rewards later on, exactly the idea of why we are doing this migration in the first place.

And to answer your questions:

do we still continue down the line of deviating this component from hooks?

No, we combine this rewrite and the one for the App component together in one ticket since it's now taking the time of three developers to do.

Excuse me if I don't exactly understand what you meant here, but last when I checked out the branch and began working on it, the component was in a weird state with half migrated hooks / having errors and needs to fix anyways, so might as well.

studioswong · 2022-02-09T11:54:33Z

on another note, @rashidakanchwala @tynandebold I am currently refactoring dropdown.js into hooks now, so no need to worry about this - I'll push the changes once it's done

studioswong · 2022-02-10T10:08:20Z

@rashidakanchwala I had pushed the latest changes in refactoring dropdown.js into react hooks, as well as updates to the test given the relevant changes.

There is still the problem with the missing 'expand more' arrow on the right of the dropdown, which was a seperate known issue to fix - I'll let you take back over for that one :)

….com:kedro-org/kedro-viz into KED-2982/rewrite-kedro-dropdown-menu-options

Signed-off-by: Tynan DeBold <[email protected]>

…D-2982/rewrite-kedro-dropdown-menu-options Signed-off-by: Tynan DeBold <[email protected]>

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold

Looking good now. Seems to work just as the old one did.

studioswong · 2022-02-15T09:59:14Z

src/components/dropdown/dropdown.js

+    if (typeof onClosed === 'function') {
+      onClosed();
+    }


This is a callback on setting the state that needs to be handled in a useEffect to ensure this is only executed after the state change (i.e the setOpen state change here).

Regardless, on checking the code, this is actually already set up in the useEffects above, hence can be deleted.

studioswong

looking good and all works the same now.

rashidakanchwala and others added 6 commits January 26, 2022 15:56

WIP

33f5d99

Signed-off-by: Rashida Kanchwala <[email protected]>

Added menu option

b3bcde5

Signed-off-by: Rashida Kanchwala <[email protected]>

Fixed styling

ebed735

Signed-off-by: Rashida Kanchwala <[email protected]>

Added tests

156b147

Signed-off-by: Rashida Kanchwala <[email protected]>

Merge branch 'main' into KED-2982/rewrite-kedro-dropdown-menu-options

08beb3b

Update release docs

71be293

rashidakanchwala marked this pull request as ready for review January 28, 2022 09:46

rashidakanchwala requested a review from yetudada as a code owner January 28, 2022 09:46

Remove unwanted code

c273b92

Signed-off-by: Rashida Kanchwala <[email protected]>

rashidakanchwala requested review from tynandebold and studioswong January 28, 2022 09:49

studioswong suggested changes Jan 31, 2022

View reviewed changes

rashidakanchwala marked this pull request as draft February 1, 2022 13:57

rashidakanchwala changed the title ~~[KED-2982] Re-write Kedro Dropdown and Menu-option~~ [KED-2982] [Do not review] Re-write Kedro Dropdown and Menu-option Feb 2, 2022

WIP - classes to hook

cebf30c

rashidakanchwala changed the title ~~[KED-2982] [Do not review] Re-write Kedro Dropdown and Menu-option~~ [KED-2982] Re-write Kedro Dropdown and Menu-option Feb 8, 2022

refactored dropdown.js to use hooks

ddc4664

studioswong and others added 6 commits February 10, 2022 14:49

removed unneccessary useRef

5471eab

Added up/down arrow

38cbd36

Merge branch 'KED-2982/rewrite-kedro-dropdown-menu-options' of github…

4fba666

….com:kedro-org/kedro-viz into KED-2982/rewrite-kedro-dropdown-menu-options

style alignments

8bdf9e6

Signed-off-by: Tynan DeBold <[email protected]>

fix console error

24ff11a

Signed-off-by: Tynan DeBold <[email protected]>

cleanup

525fb71

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold added 2 commits February 10, 2022 18:29

keyboard events

76a90fe

Signed-off-by: Tynan DeBold <[email protected]>

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

aec0bd6

…D-2982/rewrite-kedro-dropdown-menu-options Signed-off-by: Tynan DeBold <[email protected]>

tynandebold marked this pull request as ready for review February 10, 2022 18:32

tynandebold requested a review from studioswong February 10, 2022 18:32

tynandebold added 2 commits February 10, 2022 18:33

update release notes

1903616

Signed-off-by: Tynan DeBold <[email protected]>

update release notes again

883d76c

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold approved these changes Feb 10, 2022

View reviewed changes

rashidakanchwala and others added 3 commits February 14, 2022 10:01

Merge branch 'main' into KED-2982/rewrite-kedro-dropdown-menu-options

27c5c68

Merge branch 'main' into KED-2982/rewrite-kedro-dropdown-menu-options

3314533

further cleanup of comments

4097d2d

studioswong reviewed Feb 15, 2022

View reviewed changes

removed unneeded function

8ad3511

studioswong approved these changes Feb 15, 2022

View reviewed changes

Merge branch 'main' into KED-2982/rewrite-kedro-dropdown-menu-options

22f934a

studioswong merged commit cbda0d5 into main Feb 15, 2022

studioswong deleted the KED-2982/rewrite-kedro-dropdown-menu-options branch February 15, 2022 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KED-2982] Re-write Kedro Dropdown and Menu-option #721

[KED-2982] Re-write Kedro Dropdown and Menu-option #721

rashidakanchwala commented Jan 27, 2022 •

edited

Loading

studioswong left a comment

studioswong Jan 31, 2022

studioswong Jan 31, 2022

studioswong Jan 31, 2022

studioswong Jan 31, 2022

studioswong Jan 31, 2022

studioswong Jan 31, 2022

studioswong Jan 31, 2022

rashidakanchwala Feb 2, 2022

tynandebold commented Feb 8, 2022 •

edited

Loading

studioswong commented Feb 8, 2022 •

edited

Loading

rashidakanchwala commented Feb 8, 2022

tynandebold commented Feb 8, 2022

studioswong commented Feb 9, 2022 •

edited

Loading

studioswong commented Feb 9, 2022

studioswong commented Feb 10, 2022

tynandebold left a comment

studioswong Feb 15, 2022

studioswong left a comment

[KED-2982] Re-write Kedro Dropdown and Menu-option #721

[KED-2982] Re-write Kedro Dropdown and Menu-option #721

Conversation

rashidakanchwala commented Jan 27, 2022 • edited Loading

Description

Development notes

QA notes

Checklist

studioswong 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

tynandebold commented Feb 8, 2022 • edited Loading

studioswong commented Feb 8, 2022 • edited Loading

rashidakanchwala commented Feb 8, 2022

tynandebold commented Feb 8, 2022

studioswong commented Feb 9, 2022 • edited Loading

studioswong commented Feb 9, 2022

studioswong commented Feb 10, 2022

tynandebold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

studioswong left a comment

Choose a reason for hiding this comment

rashidakanchwala commented Jan 27, 2022 •

edited

Loading

tynandebold commented Feb 8, 2022 •

edited

Loading

studioswong commented Feb 8, 2022 •

edited

Loading

studioswong commented Feb 9, 2022 •

edited

Loading