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

Support native components with event handlers #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonihmig
Copy link
Contributor

Fixes #42, at least for native components.

Needs #43 and #44 to be merged first. Other than that this should be ready.

jelhan added a commit to jelhan/ember-bootstrap that referenced this pull request Jan 23, 2020
This is the result of running the tagless-ember-components-codemod
against Ember Bootstrap.

The following open pull request have been merged in before:

- Support native classes: ember-codemods/tagless-ember-components-codemod#44
- Support component with event handlers: ember-codemods/tagless-ember-components-codemod#52
- Support aria role for native classes: ember-codemods/tagless-ember-components-codemod#56

Steps:
  mv addon/templates/components/common addon/templates/components/base
  npx jelhan/tagless-ember-components-codemod#merged-native-class-support
  mv addon/templates/components/base addon/templates/components/common
@jelhan
Copy link
Contributor

jelhan commented Jan 30, 2020

Noticed a small bug when running it against Ember Bootstrap.

The event handlers are not converted to lower case. @ember/component API uses camel cased versions of the event types (e.g. keyPress) but {{on}} helper expects the event type in same spelling as it could be passed to addEventListener. So it should be keypress for the example above.

@tagName('form')
export default MyComponent extends Component {
  keyPress() {
    // ...
  }
}

is transformed to

<form {{on "keyPress" this.handleKeyPress}}>
</form>

but should be

<form {{on "keypress" this.handleKeyPress}}>
</form>

It might be enough to call toLowerCase() on eventName here: https://github.com/simonihmig/tagless-ember-components-codemod/blob/a7275a2d98834530b284fb7e6067feb81237f5e5/lib/transform/template.js#L56

@simonihmig
Copy link
Contributor Author

The event handlers are not converted to lower case

Ah, good catch, thanks @jelhan! Fixed it!

@simonihmig simonihmig changed the title [WIP] Support component with event handlers Support native components with event handlers Feb 25, 2020
@simonihmig
Copy link
Contributor Author

@Turbo87 thanks for merging the other PR. I rebased this, should be ready now as well! Don't want you to feel bored! 😉

@Turbo87
Copy link
Contributor

Turbo87 commented Mar 30, 2020

@simonihmig sorry the radio silence.

can you add this as a CLI option? maybe something like --transform-event-handlers? due to the mentioned caveats I think we probably shouldn't enable it by default.

@simonihmig
Copy link
Contributor Author

due to the mentioned caveats I think we probably shouldn't enable it by default.

@Turbo87 sorry, I might have forgotten as some time has passed, but which caveats are you referring to?

@Turbo87
Copy link
Contributor

Turbo87 commented Mar 30, 2020

but which caveats are you referring to?

I was talking about the second point of https://github.com/ember-codemods/tagless-ember-components-codemod#known-caveats

I've unfortunately seen several codebases already that did something like that :-/

Luckily, there is https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-passed-in-event-handlers.md now, that catches this issue, but probably not everyone is using that yet.

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.

Support event handlers
3 participants