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

chore(readme): explain role of ember-could-get-used-to-this better #189

Merged

Conversation

MichalBryxi
Copy link
Contributor

@MichalBryxi MichalBryxi commented Jan 2, 2022

  • Sections should be complete. If I finish installation part of
    README, I should not need to hunt down for anything that might
    be missing.
  • Be explicit: Do X, instead of generic description of what is the
    idea and leave user wondering.
  • Explicitly mentioned that it's because of templates code
  • Second commit: docs link instead of API link
    • The docs link provide nice example of the syntax
    • Hints the dot notation, which will be probably preferred in templates

- Sections should be complete. If I finish `installation` part of
  README, I should not need to hunt down for anything that might
  be missing.
- Be explicit: Do X, instead of generic description of what is the
  idea and leave user wondering.
- Explicitly mentioned that it's because of _templates_ code
README.md Outdated
@@ -18,6 +18,23 @@ npm install ember-statechart-component
yarn add ember-statechart-component
```

### Optional

To be able to use XState [`state.matches`](https://xstate.js.org/api/classes/state.html#matches)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got a little bit tangled in English sentence structure with the "which is available on the state object", so instead I just merged in the information here. It is quite a good term to grep for later on in the README, so it seems ok to me. Happy to change.

@MichalBryxi
Copy link
Contributor Author

Take all those as suggestions of a newcomer. Happy to iterate on it.

- The docs link provide nice example of the syntax
- Hints the dot notation
README.md Outdated
method in our templates,
we will first need a `HelperManager` for
handling vanilla functions.
[ember-could-get-used-to-this](https://github.com/pzuraq/ember-could-get-used-to-this)
Copy link
Owner

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 should mention this addon anymore.

There is: https://github.com/NullVoxPopuli/ember-functions-as-helper-polyfill
And in ember 4.3ish, hopefully it won't even be needed due to implementing this RFC: emberjs/rfcs#756

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, #TIL about ember-function-as-helper-polyfill. I do think however that in some form this needs to stay here, because not everyone's code will be on 4.3+. I will change to use the other addon. Once 4.3(ish) is released, the readme can be changed to give an option to skip this.

README.md Outdated

To be able to use XState [`state.matches`](https://xstate.js.org/docs/guides/states.html#state-matches-parentstatevalue)
method in our templates,
we will first need a `HelperManager` for
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe link to helper managers, too?
emberjs/rfcs#625

README.md Outdated
@@ -18,6 +18,23 @@ npm install ember-statechart-component
yarn add ember-statechart-component
```

### Optional
Copy link
Owner

Choose a reason for hiding this comment

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

Why move this under optional?

It'd be reallly awkward to use this library without the rfc 756 behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair. I understood that it's only needed for state.matches calls. Which is immensely useful. Just not necessary. But yeh, good defaults.

@MichalBryxi
Copy link
Contributor Author

I think I addressed all the comments?

@@ -18,6 +18,21 @@ npm install ember-statechart-component
yarn add ember-statechart-component
```

To be able to use XState [`state.matches`](https://xstate.js.org/docs/guides/states.html#state-matches-parentstatevalue)
Copy link
Owner

Choose a reason for hiding this comment

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

thanks for adding this! I like this as part of the installation instructions!

@NullVoxPopuli
Copy link
Owner

looks good!

@NullVoxPopuli NullVoxPopuli merged commit e0fb3ac into NullVoxPopuli:main Jan 2, 2022
@github-actions
Copy link

github-actions bot commented Jan 8, 2022

🎉 This PR is included in version 3.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants