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

Show callbacks #757

Closed
wants to merge 2 commits into from
Closed

Show callbacks #757

wants to merge 2 commits into from

Conversation

Makashov
Copy link

@Makashov Makashov commented Dec 26, 2018

Show callbacks after content item. If callback has it's own callback it will be displayed recursively.

Trying to help with #411

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 81.043% when pulling e1e8a51 on Makashov:master into ab943a8 on Rebilly:master.

@RomanHotsiy
Copy link
Member

@Makashov Thanks for the PR. It looks good overall. But I have to review it carefully and I'm a bit busy right now.

I will get to this ASAP. Sorry for the delay.

@Makashov
Copy link
Author

Thank you for your response.
I have enough time so, if you have any comments or recomendations I am ready to fix them.

@nupejosh
Copy link

Hey @Makashov I used the Open API v3.0 example provided here with the master branch of your fork, but I'm not able to see the callbacks details in the Redoc UI. Is there a different format that I should be using for your fix to work?

@Makashov
Copy link
Author

Hi @nupejosh, I just added summary to {$request.query.callbackUrl}/data callback object and it looks like below. Rigth after responses there must be callbacks list.

image

@nupejosh
Copy link

Thanks for the response @Makashov! I see it working perfectly for you there. Sorry for the openapi noob request man, but can you paste the yaml or json where you added the summary to the callback object? For some reason I can't get the Callbacks block to show in the UI. I'm sure I'm missing something simple, but I've been trying for a couple days now :/

@laurence-myers
Copy link

laurence-myers commented Jun 19, 2019

This PR has been idle for a while. What's needed to move things along? I see these things need to be addressed:

  • Merge from master, address merge conflicts
  • Add tests to avoid lowering code coverage
  • Remove unrelated TS formatting changes (this might just be because this branch has fallen behind master)
  • Provide an example YAML for manual testing

Should I raise a new PR with these changes?

@RomanHotsiy
Copy link
Member

RomanHotsiy commented Jun 20, 2019

There are lots of PRs idle for a while 😢.
I just have little time to review them all... I hope I will find some time soon to start reviewing all the PRs.

About this PR, I checked it quickly. I'm not quite sure about how it should be presented.

This PR makes a horizontal line under the operation and adds callback there which I think may be confusing...

We should investigate the options. Should we add callbacks to the left menu? Should they visually be a part of the operation or a separate item?

You're welcome to experiment in a new PR.

@zucher
Copy link

zucher commented Jul 17, 2019

Hi @RomanHotsiy ,

How to help in order to progress in this feature?

Thank you

zucher

@bsramin
Copy link

bsramin commented Oct 4, 2019

any news @RomanHotsiy
we can help?

URL: <a href={info.contact.url}>{info.contact.url}</a>
</InfoSpan>
)) ||
(info.contact && info.contact.url && (
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is because of prettier update which improved formatting a bit

@Jonathan-Bailey-Bose
Copy link
Contributor

I have created a branch (pull #1102) off of this one in a fork in order to resolve merge conflicts, write tests and add some minor functional differences to the existing code in this pull request. @Makashov I have credited your work accordingly, if there is any issue you have with this please let me know, or if you'd like to pull my changes/additions into this branch in order to try to expedite the process of getting this feature merged, that works too.

@RomanHotsiy
Copy link
Member

Closing in favour #1102.

@Makashov sorry for the delay. I'm super busy last year and this is not so simple feature to just merge. It needs careful review.

I hope I will review @Jonathan-Bailey-Bose PR faster.

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

Successfully merging this pull request may close these issues.

9 participants