Skip to content

Conversation

ellinge
Copy link
Collaborator

@ellinge ellinge commented Mar 31, 2019

What does it do?

When adding typings in #223 it was found that the current implementation of onAction is not working.
Because of "deepcloning" the local onAction-events does not work anymore as intended.

The global event worked though but was somewhat limited. The only thing passed was the generated actionid (if not set as prop) and nodeid. See:
bild

To be consistent with the other events the global action is now changed to pass the action-object and current node instead, or defined with TypeScript instead:

onChange?: (currentNode: TreeNode, selectedNodes: TreeNode[]) => void;
onAction?: (currentAction: NodeAction, currentNode: TreeNode) => void;
onNodeToggle?: (currentNode: TreeNode) => void;

Where NodeAction and TreeNode is:


Type of change

  • Breaking change
  • This change requires a documentation update

@coveralls
Copy link

coveralls commented Mar 31, 2019

Pull Request Test Coverage Report for Build 1141

  • 5 of 5 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 94.403%

Totals Coverage Status
Change from base Build 1128: -0.08%
Covered Lines: 566
Relevant Lines: 581

💛 - Coveralls

@ellinge
Copy link
Collaborator Author

ellinge commented Mar 31, 2019

Up and running under events here:
https://ellinge.github.io/react-dropdown-tree-select-test/#DevelopTemp-onaction

@mrchief
Copy link
Collaborator

mrchief commented Apr 1, 2019

Looks good!

Since it's a breaking change, I guess we need to update the title to reflect that so that versions are bumped accordingly.

Also, should we provide a migration guide about new action syntax? It's not much but usually is expected for a major version bump.

@ellinge
Copy link
Collaborator Author

ellinge commented Apr 1, 2019

Since it's currently not working on local and you only get passed the action/node id I guess it would be something like this:

Migration step onAction

If you previously passed:

{ id: 'mynode' myCustomNode: true, actions: [ { id: 'myaction', myCustomAction: true ... } ... }

which you accessed on the onAction event like:

onAction = ({ action, id }) => { console.log(action, id) }

you can now access the same info with:

onAction = (action, node) => { console.log(action.id, node.id) }

In addition, all custom props you pass, such as myCustomNode and myCustomAction, is now also accessible on the event properties. Making it easier to add generic custom logic based on your custom data instead.

Where should this migration step be added?

@mrchief
Copy link
Collaborator

mrchief commented Apr 2, 2019

Since it's currently not working on local and you only get passed the action/node id

It'd be working for those on v1.0.4 or older. Hence the dilemma.

Where should this migration step be added?

How about docs/guides/migrating-from-1-to-2.md? And link it via the PR's description (not sure about this part but doing a dry-run with semantic-release will tell us more).

@ellinge
Copy link
Collaborator Author

ellinge commented Apr 2, 2019

Since it's currently not working on local and you only get passed the action/node id

It'd be working for those on v1.0.4 or older. Hence the dilemma.

Where should this migration step be added?

How about docs/guides/migrating-from-1-to-2.md? And link it via the PR's description (not sure about this part but doing a dry-run with semantic-release will tell us more).

Tried v1.04 here but can't get it working onLocal either. I can see that the onGlobal-properties is different from v1.17 as well.
https://ellinge.github.io/react-dropdown-tree-select-test/#v104-onaction

Perhaps it's better you write the migration step since you have the history of it's previous workings?

@mrchief
Copy link
Collaborator

mrchief commented Apr 3, 2019

Tried v1.04 here but can't get it working onLocal either. I can see that the onGlobal-properties is different from v1.17 as well.
ellinge.github.io/react-dropdown-tree-select-test/#v104-onaction

Thanks for taking the effort. It could be buggy from beginning. We can check but I think it'll be a fruitless exercise.

Perhaps it's better you write the migration step since you have the history of it's previous workings?

Your description is pretty accurate but I'll let you know if I find anything.

@ellinge
Copy link
Collaborator Author

ellinge commented Apr 15, 2019

I've added a migration doc now.

@mrchief mrchief changed the title feat: Updated onAction to pass objects BREAKING CHANGE: Updated onAction to pass objects Apr 22, 2019
@mrchief
Copy link
Collaborator

mrchief commented May 8, 2019

@ellinge Made some tweaks - primarily to make onAction signature consistent with remaining handlers.

Also, I think there is a way to include the migration steps in the release log itself. Needs some experimentation to be sure.

@mrchief mrchief changed the title BREAKING CHANGE: Updated onAction to pass objects fix: Updated onAction to bubble up custom props May 8, 2019
@mrchief
Copy link
Collaborator

mrchief commented May 8, 2019

Confirmed. We don't need a separate guide. I can add the breaking change description in the release log itself.

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 09a624f and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Collaborator

@mrchief mrchief left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for sending this!

@mrchief mrchief merged commit aee6e7a into develop May 9, 2019
@mrchief mrchief deleted the fix/OnAction branch May 9, 2019 13:51
mrchief pushed a commit that referenced this pull request Jun 10, 2019
BREAKING CHANGE: Action Changes

- The option to pass a local `onAction` handler on a node is now removed. Use the **global** `onAction` event instead.

  ```jsx
  <DropdownTreeSelect onAction={onAction} ... />
  ```

- `onAction` signature is now consistent with signature for other event handlers such `onChange` and `onNodeToggle`

  ```js
  // before
  onAction = ({ action, id }) => {
    console.log(action, id)
  }

  // after
  onAction = (node, action) => {
    console.log(action, node.id)
  }
  ```

- Any custom props passed to `node` or `action` is accessible in the event properties. This will make it easier to add generic custom logic based on your custom data/properties which can be used instead of separate handlers.

  For example:

  ```js
  // node with action and custom props
  {
    id: 'mynode',
    propA: 'hello',
    propB: true,
    actions: [
      {
        id: 'myaction',
        propX: {...},
        propY: 12
      }
    ]
  }

  onAction = (node, action) => {
    console.log(node.propA, node.propB, action.propX, action.propY)
    // prints hello true {...} 12
  }

  ```
mrchief pushed a commit that referenced this pull request Jun 10, 2019
BREAKING CHANGE: Action Changes

- The option to pass a local `onAction` handler on a node is now removed. Use the **global** `onAction` event instead.

  ```jsx
  <DropdownTreeSelect onAction={onAction} ... />
  ```

- `onAction` signature is now consistent with signature for other event handlers such `onChange` and `onNodeToggle`

  ```js
  // before
  onAction = ({ action, id }) => {
    console.log(action, id)
  }

  // after
  onAction = (node, action) => {
    console.log(action, node.id)
  }
  ```

- Any custom props passed to `node` or `action` is accessible in the event properties. This will make it easier to add generic custom logic based on your custom data/properties which can be used instead of separate handlers.

  For example:

  ```js
  // node with action and custom props
  {
    id: 'mynode',
    propA: 'hello',
    propB: true,
    actions: [
      {
        id: 'myaction',
        propX: {...},
        propY: 12
      }
    ]
  }

  onAction = (node, action) => {
    console.log(node.propA, node.propB, action.propX, action.propY)
    // prints hello true {...} 12
  }

  ```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants