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

Add ember-modifier dependency #1

Closed
wants to merge 2 commits into from
Closed

Conversation

SergeAstapov
Copy link
Owner

No description provided.

Copy link

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Great start on this! I had some suggestions, ranging from fixing typos and grammar to adding more text entirely. Let me know once you've incorporated this!

text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
text/0000-ember-modifier.md Outdated Show resolved Hide resolved
@SergeAstapov
Copy link
Owner Author

Great start on this! I had some suggestions, ranging from fixing typos and grammar to adding more text entirely. Let me know once you've incorporated this!

Thank you for suggestions! I've applied them all and made some tweaks adding more prose to Motivation section.

Integrate ember-modifier into the @ember/modifier package directly.

BTW this alternate option seem logical to me.
I wonder if there are strong arguments why we should not be doing exact same thing?

@chriskrycho
Copy link

BTW this alternate option seem logical to me. I wonder if there are strong arguments why we should not be doing exact same thing?

We might! I’ll chat about it with folks at the Framework team meeting this week. We also might do it via @glimmer/modifier. We just need to align on the best thing design-wise!

Copy link

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Really nice expansion! One more suggested round of tweaks, and then you should open it against the Ember RFCs repo. Thanks for your work on this!

text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
text/0000-element-modifier.md Outdated Show resolved Hide resolved
@SergeAstapov
Copy link
Owner Author

@chriskrycho thank you for the great suggestions! They have been applied and PR opened emberjs#811

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