Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify action event handler #14872

Merged
merged 1 commit into from
Jan 25, 2017

Conversation

GavinJoyce
Copy link
Member

@GavinJoyce GavinJoyce commented Jan 24, 2017

As @rwjblue notes, the pre-glimmer 2 implementation had a single data-ember-action="someid" and multiple actions would be registered on that id.

Glimmer 2 has a unique action id per action registration so we no longer need to store the registered actions in an array.

TODO:

@GavinJoyce GavinJoyce changed the title [wip] further simplify event handler [wip] further simplify action event handler Jan 24, 2017

actions = [];
if (attrName.indexOf('data-ember-action-') === 0) {
//TODO: GJ: why does `registeredActions` contains arrays? are they always of length 1?
Copy link
Member

Choose a reason for hiding this comment

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

In the pre-glimmer2 implementation it was an array because we only tagged the element with data-ember-action="someid" and that element could have multiple actions:

<button {{action 'foo' on="click"}} {{action 'bar' on="doubleClick"}}></button>

In the glimmer 2 implementation, we have maintained it as an array but it can only ever have one entry.

Some references:

I think that registerAction can be simplified to something like:

  registerAction(actionState) {
    let { actionId } = actionState;

    ActionManager.registeredActions[actionId] = actionState;

    return actionId;
  },

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the context, I'll update registerAction to remove the arrays and simplify the action handler


actions = [];
if (attrName.indexOf('data-ember-action-') === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we only care about the attribute starting with the given string, I think you could do:

if (attrName.lastIndexOf('data-ember-action-', 0) !== -1) {

}

if (actionId === '') {
let attributes = evt.currentTarget.attributes;
let attributeCount = attributes.length;
for (let i = 0; i < attributeCount; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to cache the length here (we are not mutating AFAICT).

@rwjblue
Copy link
Member

rwjblue commented Jan 24, 2017

ready for a rebase also

@GavinJoyce GavinJoyce force-pushed the gj/further-simplify-event-handler branch from 6107018 to b327e87 Compare January 24, 2017 20:28
@GavinJoyce GavinJoyce changed the title [wip] further simplify action event handler Simplify action event handler Jan 24, 2017
@GavinJoyce
Copy link
Member Author

@rwjblue thanks for the feedback, this is 🍏 now.

It appears that only one addon uses ActionManager.registeredActions:


if (action && action.eventName === eventName) {
return action.handler(evt);
if (action.eventName === eventName) {
Copy link
Member Author

Choose a reason for hiding this comment

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

is it ok to remove the if(action && guard here? Do we care about supporting non existent actions?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably fine. I can't see how we would end up here with an action that is falsey without having already screwed something majorly up elsewhere.


if (action && action.eventName === eventName) {
return action.handler(evt);
if (action.eventName === eventName) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably fine. I can't see how we would end up here with an action that is falsey without having already screwed something majorly up elsewhere.

@rwjblue rwjblue merged commit fc729c9 into emberjs:master Jan 25, 2017
@GavinJoyce GavinJoyce deleted the gj/further-simplify-event-handler branch January 25, 2017 17:34
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.

2 participants