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

Update modifiers related sections to be accurate to what we've been able to do since 3.25 #1829

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jul 13, 2022

This will

  • Update existing modifier language in the release docs
  • once approved, I'll back port it all the way to 3.25
  • removes mentions of @ember/render-modifiers as those modifiers were meant for migration from the old paradigms, and not exactly reflective of how the Octane mental model works.

Prior to 3.25, all modifiers had to be globally available in app/modifiers/* -- this is very awkward, and kind of unfortunate from a teaching perspective that we had such a big gap between modifiers being "the thing" and when they can be freely used -- so I'll be ignoring everything prior to 3.25 for these updates.

Copy link
Contributor

@jenweber jenweber left a comment

Choose a reason for hiding this comment

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

I chatted with nullvoxpopuli, and I can help finish up this PR. I have some suggestions that I will apply directly and then request review.

@jenweber jenweber requested a review from a team July 23, 2022 15:08
@jenweber
Copy link
Contributor

I made some small wording changes, added a label to the pre-existing input example, and added eager: false to address a deprecation warning. I tested this example and I think it's good to go. Would anyone else like to review before we merge? @ember-learn/learning-core-team

@NullVoxPopuli
Copy link
Contributor Author

hmm eager: false "feels temporary". 🤔 @chriskrycho -- any feels as to when ember-modifier v4 is happening? <3

<input {{did-insert this.focus}}>
</form>
```
npx ember install ember-modifier
Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, this should be ember-cli

this package: https://www.npmjs.com/package/ember
only prints this message: https://github.com/emberjs/ember/blob/master/index.js

and doesn't actually do anything.
Would be great if ember were configured as a bin that delegated to npx ember-cli

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, thank you. I will change it to use global ember for now.

<input {{did-insert this.focus}}>
</form>
```
npx ember install ember-modifier
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
npx ember install ember-modifier
npx ember-cli install ember-modifier

@NullVoxPopuli
Copy link
Contributor Author

I just found -- guides/release/upgrading/current-edition/glimmer-components.md / https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/

I'm gonna address in a separate PR once we resolve this one 🥳

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