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

@required vs required #146

Closed
Turbo87 opened this issue Oct 1, 2019 · 7 comments · Fixed by #152
Closed

@required vs required #146

Turbo87 opened this issue Oct 1, 2019 · 7 comments · Fixed by #152
Labels
bug Something isn't working

Comments

@Turbo87
Copy link
Collaborator

Turbo87 commented Oct 1, 2019

The codemod is currently converting required arguments to required attributes, instead of @required arguments. This might be fine for <Input> (is it?), but certainly not for all components.

The same applies for placeholder vs. @placeholder.

The codemod also does something similar for class, but I suspect in that case it should be fine.

@rwjblue
Copy link
Member

rwjblue commented Oct 1, 2019

Ya, we should fix this. We can build in assumptions about Ember's built-in components, but we cannot make any assumptions about the others.

tldr; IMHO, this codemod should not be migrating away from arguments to attributes in non-built-in components.

@rwjblue rwjblue added the bug Something isn't working label Oct 1, 2019
@Turbo87
Copy link
Collaborator Author

Turbo87 commented Oct 1, 2019

@rwjblue how do you feel about class?

Copy link
Member

rwjblue commented Oct 1, 2019

It depends on what level of "safety" we are shooting for. class is absolutely a value that folks might use and munge in their JS, and assuming that their JS does not need access to it is conceptually breaking.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Oct 1, 2019

I had similar thoughts but then couldn't come up with a realistic scenario where it actually mattered for the class attribute. But I'm fine either way. the safest way is definitely to pass it as an argument instead of attribute.

@tylerturdenpants
Copy link
Collaborator

SO as an example, it should not do:

{{foo-bar class="baz"}}

to

<FooBar class="baz"/>

but rather

<FooBar @class="baz"/>

correct?

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Oct 1, 2019

@tylerturdenpants yeah, exactly. except maybe for builtin components like input and textarea.

@tylerturdenpants
Copy link
Collaborator

Ok. Crystal clear now. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants