Skip to content

avoid sendAction when possible#825

Closed
juanazam wants to merge 1 commit intoAddepar:masterfrom
juanazam:remove_send_action_deprecations
Closed

avoid sendAction when possible#825
juanazam wants to merge 1 commit intoAddepar:masterfrom
juanazam:remove_send_action_deprecations

Conversation

@juanazam
Copy link

@juanazam juanazam commented Jun 6, 2020

Hello!

Been an ember-table user for quite some time, so I thought it was nice to give back to the community.

Closes #614

Used addon suggested by @pzuraq on the issue.
Kept backwards compatibility for 1.12 users.

johanrd added a commit to johanrd/ember-table that referenced this pull request Jul 31, 2020
johanrd added a commit to johanrd/ember-table that referenced this pull request Dec 8, 2020
@mixonic mixonic added the 3.0 label Jan 11, 2021
@mixonic mixonic mentioned this pull request Jan 11, 2021
@mixonic
Copy link
Member

mixonic commented Jan 11, 2021

The non-1.13 branch of this code is likely what should land into the 3.0-beta branch before the final 3.0 is cut. I'm not sure this will end up as the PR, but flagged it 3.0 regardless since it should be addressed by then.

@juanazam
Copy link
Author

@mixonic do you want me to do any changes? I can help, just let me know, I can help with 3.0 in general as well

@twokul
Copy link
Contributor

twokul commented Feb 1, 2021

@juanazam Thanks for the PR! If you have free cycles this week to work on it, that'd be great.

A few thoughts after looking at the code:

  • I think we should target this PR against 3.0-beta
  • We should not add ember-compatibility-helpers dependency (all the checks for 1.13+ should go away too, closure actions is the default behaviour)
  • We still have methods that start with sendAction (sendEventAction, sendFullAction); we should likely rename them to something like callClosureAction
  • We need to consider cases when a closure action is not defined (to prevent errors like undefined is not a function)
  • There are a couple of cases when we pass a bound sendAction function (here and here); we can no longer do that. We are going to have to be explicit about what type of actions both of those objects can call (ColumnTree, CollapseTree)

I'm happy to keep collaborating on this PR 😄

@juanazam
Copy link
Author

juanazam commented Feb 1, 2021

@twokul I understand all points listed but the last one

You mean renaming sendAction to something else? or something more complex than that?

Cheers!

@twokul
Copy link
Contributor

twokul commented Feb 1, 2021

@juanazam I think the following (I am not 100%):

this.columnTree = ColumnTree.create({
- sendAction: this.sendAction.bind(this),
+ onSelect: this.onSelect.bind(this),
+ onReorder: this.onReorder.bind(this),
  columnMetaCache: this.columnMetaCache,
  containerWidthAdjustment: this.containerWidthAdjustment,
});

Or

this.columnTree = ColumnTree.create({
- sendAction: this.sendAction.bind(this),
+ onSelect: () => this.onSelect?.(),
+ onReorder: () => this.onReorder?.(),
  columnMetaCache: this.columnMetaCache,
  containerWidthAdjustment: this.containerWidthAdjustment,
});

@twokul twokul mentioned this pull request Feb 2, 2021
12 tasks
@twokul
Copy link
Contributor

twokul commented Feb 2, 2021

@juanazam We really appreciate your work on this PR but looking to make progress as fast as possible. I'm going to close this PR in favour of #860.

@twokul twokul closed this Feb 2, 2021
@juanazam
Copy link
Author

juanazam commented Feb 2, 2021

@twokul sounds good! suoer busy this week!

@juanazam juanazam deleted the remove_send_action_deprecations branch February 2, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

sendAction deprecation

3 participants