-
-
Notifications
You must be signed in to change notification settings - Fork 638
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: adding dynamic channel page #1959
Conversation
✅ Deploy Preview for shimmering-choux-eb0798 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
time to work, Miss Mhafuzaaaa ✨✨✨✨✨ 😄✌🏽
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 gave my best providing an exhaustive review. However, I did not write all the things I found that need a change.
I feel this document needs a deep rewrite, specially regarding the "tone" of the message we give; it feels very marketing/sales oriented instead of a technical document.
Listing few sentences as example:
The Parameter Context clarifies the origin of parameters, ensuring consistent understanding across teams. It also enables efficient code generation, thereby accelerating development and enhancing the developer experience.
are pivotal for enhancing the flexibility, interoperability, reusability, and standardization of asynchronous APIs.
This adaptability to different APIs aids in creating a unified view of the API, making it easier to comprehend and utilize.
I will always advocate for short, concise and focused on explain concepts. Skipping fancy words and using just plain English instead.
If you need info about what the feature does, and you feel the current documentation in the Specification Reference is not enough, I'm happy to jump into a call or create a video, etc.
I will change the file name later as if I change it now, all reviews which I havent fixed yet, are gonna be outdated.
@mhmohona you can now easily validate your AsyncAPI v3 docs with https://studio.asyncapi.com. here is how to enable it https://www.loom.com/share/0bab6489e24c4e03b0f7951ed793a240?sid=d98f7310-3d0d-4a4c-ac1f-96e4247d5b74 |
@smoya thank you for your feedback. I have addressed your review and updated accordingly. Can you please have a look at it again? |
@mhmohona Please review this PR and ensure you resolve EACH feedback comment left. Sergio left a lot of extremely useful and detailed feedback, but you didn't resolve each item and respond to each one. Let's do that now before we bother Sergio or others for another review because it is hard to tell what feedback you addressed/missed. ✌🏽😄 |
pages/docs/concepts/asyncapi-document/dynamic-channel-address.md
Outdated
Show resolved
Hide resolved
pages/docs/concepts/asyncapi-document/dynamic-channel-address.md
Outdated
Show resolved
Hide resolved
pages/docs/concepts/asyncapi-document/dynamic-channel-address.md
Outdated
Show resolved
Hide resolved
B --> D | ||
D --> E | ||
D --> F | ||
``` |
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.
As mentioned in #1959 (comment), I still believe this diagram doesn't clarify anything to the user but rather can lead to confusion. I would suggest we remove it.
userSignedUp: | ||
address: 'user.signedup' | ||
messages: | ||
userSignedUp: | ||
$ref: '#/components/messages/userSignedUp' |
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.
This example is not showing any parameter in the address. You are already providing one example below in https://github.com/asyncapi/website/pull/1959/files#diff-798204f3a6dff3c49407fb1af6e8d8d4a10a5b26e328844fe1e03e17173e3c98R66.
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.
So here shall I remove the code example? Or I should provide a new example?
F --> G | ||
A --> G | ||
|
||
style B fill:#47BCEE,stroke:#47BCEE; |
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.
As mentioned in #1959 (comment), I still believe these diagrams don't clarify anything to the user but rather can lead to confusion. I would suggest we remove it.
pages/docs/concepts/asyncapi-document/dynamic-channel-address.md
Outdated
Show resolved
Hide resolved
```mermaid | ||
graph TD | ||
subgraph Channels | ||
userSignedUp["userSignedUp"] | ||
end | ||
|
||
subgraph Parameters | ||
userId["userId"] | ||
end | ||
|
||
userSignedUp -->|requires| userId | ||
|
||
subgraph Reused Parameters | ||
UserUpdated["UserUpdated"] | ||
style UserUpdated fill:#47BCEE,stroke:#47BCEE; | ||
|
||
end | ||
UserUpdated -->|reuses| userId | ||
``` | ||
|
||
In this diagram, a channel named `userSignedUp` that requires a parameter `userId` .Additionally, the parameter `userId` is reused for another message, `UserUpdated`. |
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.
As mentioned in #1959 (comment), I still believe these diagrams don't clarify anything to the user but rather can lead to confusion. I would suggest we remove it.
The example below is more than enough to illustrate
Description
Added Dynamic channel name page.
It is a part of GSoD'23 project.
Related issue(s): fixes ##1511