-
Notifications
You must be signed in to change notification settings - Fork 406
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
docs: announce version 17 #2065
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit de99dad. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
BundleMon (Integration Projects)Unchanged files (2)
No change in files bundle size Final result: ✅ View report in BundleMon website ➡️ |
0c86e73
to
de99dad
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 370ea7f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 4 targetsSent with 💌 from NxCloud. |
d0ff7dd
to
0cebaa9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback from a quick review
|
||
NGXS provides a robust error handling mechanism that automatically catches unhandled exceptions thrown within actions. These errors are then directed to a centralized `ErrorHandler`. | ||
|
||
However, you can override the default implementation and provide your custom one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, you can override the default implementation and provide your custom one | |
However, you can override the default implementation and provide your custom one. |
|
||
--- | ||
|
||
## Plugin Improvements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that plugin improvements should be placed at the end. This is because it describes that some of the symbols are now privately exported, which is less prioritized compared to other information we're providing in these docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point! I moved it just before the NGXS Community
section. My thought was to have the NGXS community
section as the closure.
Do you think the Plugin Improvements
should be the last section of the readme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have the discord link at the very top, like 'first of all, we're moving to discord' or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things about the Discord question
- Is this "move" to Discord in relation to Slack being the default currently (from what I can tell at the moment)? I joined the Slack recently as it seemed to be pinned to the top of the
README.md
. If so, in my opinion I definitely would highlight the move to Discord towards the top of this announcement post, or at the least not at the very bottom. - If I am correct in this assumption, make sure to update the
README.md
badge as well as I assume the NGXS Labs contact part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment @michael-small
Indeed makes sense to move that on top. As of your second point, the README.md
file has been updated and currently is part of the development version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions about this version, reading this as if I had no context with what is going on now. Technically I don't know as much anyways, but I know I have had the pleasure of meeting up with some team members recently so I am reading this article with some more context than most other users who will read this post.
- The article declares that this is NGXS 17, but the branch name is announcing 3.9. Are versions from NGXS 17+ going to be 1:1 with Angular majors? If so, in my opinion I would highlight this in the article.
- I don't know what the compatibility matrix of Angular versions to NGXS would be with NGXS 17, but if it is different than previous version upgrades I would also call this out in the announcement. At least the minimum version to use the standalone and signal changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes!
@michael-small makes very good points. Could you add a paragraph at the beginning about aligning versions with Angular, in order to readily take advantage of the latest features as the framework continues to evolve.
And a mention of the minimum version required for the signals and standalone support.
Code Climate has analyzed commit 370ea7f and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 95.3% (0.0% change). View more on Code Climate. |
WIP
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information