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: add parameter section in migration #2293

Merged

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Nov 6, 2023

Description
This PR adds the missing parameter section that gives an overview of the new parameter object.

I also decided to remove the initial interactive comparison, as with the number of changes it simply became so cluttered that it was no longer possible to get anything meaningful out of it...

I also added a small section on the messageId that is now removed.

Copy link

netlify bot commented Nov 6, 2023

Deploy Preview for shimmering-choux-eb0798 ready!

Name Link
🔨 Latest commit ad9be7a
🔍 Latest deploy log https://app.netlify.com/sites/shimmering-choux-eb0798/deploys/6557d168784e210009f4d61e
😎 Deploy Preview https://deploy-preview-2293--shimmering-choux-eb0798.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

derberg
derberg previously approved these changes Nov 7, 2023
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

technically accurate

@alequetzalli please have a look

@jonaslagoni
Copy link
Member Author

Thanks @alequetzalli 👍

@jonaslagoni
Copy link
Member Author

/rtm

@quetzalliwrites
Copy link
Member

lol slow your roll, @jonaslagoni, this PR needs 2 approvals to merge

since I added a few commits, that automatically re-requests reviewers to approve PR again... you need @derberg (or another code owner) to hit approve again

@jonaslagoni
Copy link
Member Author

If anything is missing i.e. approves it wont do anything but let them know that I see it as being ready to be merged 🙂 So yea now it's up the codeowners to merge it 🙂

@quetzalliwrites
Copy link
Member

I'm actually going to merge any of these v3 PRs that I'm able to, I know these are ready to merge cause I approved them. lol duh 😂😂😂😂

But like you saw, I cannot merge the PRs I make commits to cause our repo has permissions set up as defined above. Breathe dude, these are all getting merged for the release and Lukasz and I are working together as needed to move them forward.

@akshatnema
Copy link
Member

If anything is missing i.e. approves it wont do anything but let them know that I see it as being ready to be merged 🙂 So yea now it's up the codeowners to merge it 🙂

Yo man! I got your notification at 3 AM at night 😄. So, I'll look into PR tomorrow. Sorry, it got missed off in previous notifications.

@quetzalliwrites
Copy link
Member

oooh @akshatnema this is like SO ready to go, can you help him by approving? we don't really need another review, it's already approved by lukasz and I

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Nov 10, 2023

There are no rush here folks 😆 It happens when it happens, I am just using /rtm as soon as it looks like the PR is ready to be merged from my side.

No need to freak out over it, I just do it so we are at least not waiting on me in any sense 😆 Which have happened in the past where an approval was given but nobody did the /rtm

@akshatnema
Copy link
Member

akshatnema commented Nov 10, 2023

oooh @akshatnema this is like SO ready to go, can you help him by approving?

Yeah, I looked into the PR preview. I'm quite suspicious that the comparison that is created is not working well in the mobile view. I was touching repeatedly on the sections, but they haven't been highlighted. That's why, I'll look into the PR tomorrow, from my laptop. And will do some manual testing.

@quetzalliwrites
Copy link
Member

oh interesting! thanks for helping catch issues @akshatnema, appreciate you! :)

@akshatnema
Copy link
Member

Hey @jonaslagoni, I'm not able to get any hover effect on this component. Why?

image

Also, in the following component, I'm not able to get hover activity at schema field.

image

The following fields are automatically colored, without hovering into them:

image

Copy link
Member

@akshatnema akshatnema left a comment

Choose a reason for hiding this comment

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

Comments added above to request some changes.

@jonaslagoni
Copy link
Member Author

Hey @jonaslagoni, I'm not able to get any hover effect on this component. Why?

Back in the first PR I didnt really see what to compare, so I never added any effects there.

Also, in the following component, I'm not able to get hover activity at schema field.

Fixed that one 👍

The following fields are automatically colored, without hovering into them:

Because they are removed and no longer available 🙂

@fmvilas fmvilas mentioned this pull request Nov 24, 2023
13 tasks
@akshatnema
Copy link
Member

/rtm

@akshatnema
Copy link
Member

@alequetzalli Can you please re-approve this PR? Your preview review got dismissed.

Copy link
Member

@quetzalliwrites quetzalliwrites left a comment

Choose a reason for hiding this comment

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

/rtm

@quetzalliwrites
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit c77af4d into asyncapi:next-major-spec Nov 29, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants