-
-
Notifications
You must be signed in to change notification settings - Fork 651
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: add v3 migration guide #2008
docs: add v3 migration guide #2008
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.
Hi @jonaslagoni, Thanks for initiating this PR. The new doc page for migration to v3 is looking great. Here are some of my concerns I think you should consider in mobile view.
If we are following comparison boxes to be ordered in a vertical fashion, it should be followed all over the page. So, just update this v2 & v3 comparison to be in vertical alignment in mobile view, as you did for others.
There should be some minimum gap between these two comparison boxes in mobile view. Kindly add a gap
property in them if you have made them using flexbox. Applicable everywhere.
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've reviewed the files as per my capabilities. Please have a look and provide your feedback 🙏 @jonaslagoni @alequetzalli @thulieblack
Co-authored-by: Animesh Kumar <kumaranimesh923@gmail.com>
Co-authored-by: Animesh Kumar <kumaranimesh923@gmail.com>
@akshatnema do you have a suggestion as to how the class names need to change to satisfy the design you are looking for? |
@AnimeshKumar923 thanks for the feedback, applied most of the suggestions, some needs to be discussed 🙂 |
Okay. Let me know, how to discuss these... |
@AnimeshKumar923 in your comments 🙂 I left some further suggestions and in #2008 (comment) I cant come up with anything better, so hoping @alequetzalli will jump in 😄 |
Will look into it later 👍🏻 |
@jonaslagoni Should I directly commit the changes or made them as suggestions inside this PR? |
@akshatnema would you mind having a look? 🙏 |
Please have another look @akshatnema 🙂 |
After this is merged I am gonna add the parameters section. |
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 ✔️
@alequetzalli we're just missing your approval I think |
Hey @derberg, I committed a rewrite here -- this doc is amazing, but still needed several fixes. Now we're good to merge. ✌🏽 |
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.
🪄✨✨
@alequetzalli thanks a lot @jonaslagoni you wanna have a final look after rewrite from Alejandra? sometimes such rewrite/refactor can affect the message (there are no unit tests for docs 😄 ), so just wanna double check if you wanna have a look before merging |
LGTM 👍 |
/rtm |
@akshatnema think this needs you approval, or @derberg |
/rtm |
/rtm |
Description
This PR is a replacement for #660
This PR adds a migration page for version 3 that shows the difference between v2 and v3 in an interactive manner.
Related issue(s)
Related to asyncapi/spec#691