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

Multiple examples #4092

Closed
wants to merge 3 commits into from
Closed

Conversation

ffissore
Copy link

This is a re-work of #3616
All commits made by @biggates were squashed into one commit, put on top of current master (= 3.9.0), and cleaned up so that npm test passed
3 additional commits were added to clean up the code a bit more, and to remove runtime errors

How Has This Been Tested?

We are about to deploy it to production, but no automatic tests were added

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@shockey shockey self-requested a review January 16, 2018 03:08
@ffissore ffissore force-pushed the ft/multiple-examples branch 2 times, most recently from f081390 to 889ad9b Compare January 16, 2018 09:12
@ffissore
Copy link
Author

Removed dist files commit and rebased

@theangrydev
Copy link

When is this due to be released?

@heldersepu
Copy link
Contributor

@theangrydev There are conflicts on this PR
Safe to assume that those have to be fixed before it will be considered...
Looking at the code changes it looks simple, once the conflicts are corrected it should move fast.

@ffissore consider adding some UnitTests

@ffissore ffissore force-pushed the ft/multiple-examples branch 2 times, most recently from b4a3246 to 5378801 Compare February 26, 2018 07:06
@ffissore
Copy link
Author

I've rebased the PR. I'm sorry but I'm not a react dev: all the work has been done by @biggates, I just managed to clean up the code. I don't know where and how to add unit tests.

@ffissore ffissore force-pushed the ft/multiple-examples branch 2 times, most recently from 3cc992a to c07a04b Compare March 6, 2018 12:16
@JohneDoe
Copy link

why is it not merged?

@heldersepu
Copy link
Contributor

Hey @JohneDoe if you have some time please add some UnitTests

@niklasnorin
Copy link

Can't wait for this to be merged!

@havwig
Copy link

havwig commented Mar 25, 2018

Is there anything I can do to help this along?

@stbischof
Copy link

@havwig I think only UnitTests are needet and merging the new commits. For me its the wrong language. But I'm really interested in this Feature!

@webron
Copy link
Contributor

webron commented Apr 2, 2018

See #3803 for related discussion.

@jwalton
Copy link

jwalton commented Apr 9, 2018

What unit tests does this need? Can I do something to help move this along?

@ffissore ffissore force-pushed the ft/multiple-examples branch 2 times, most recently from b7044ae to e8fb25f Compare April 11, 2018 12:45
@mika018
Copy link

mika018 commented May 3, 2018

Any chance of resolving the conflicts soon? Thanks

@ffissore
Copy link
Author

ffissore commented May 4, 2018

Rebased once more, although likely to stop doing so as this PR didn't get enough traction.
Also please be aware that we are not using this latest version in production: we are still using its first release, based on swagger-ui 3.9.0 as stated in PR first comment

@ffissore
Copy link
Author

ffissore commented May 4, 2018

If you know react, please add unit tests where appropriate. That will help this PR

@webron
Copy link
Contributor

webron commented May 4, 2018

@ffissore conceptually, the PR is important to us. The main problem is with the UX and that's something we have to explore and verify before merging anything in. I'm hoping to get some people to help review the UX aspect in a few weeks - this is a bit more challenging than other changes to the UI.

@ffissore
Copy link
Author

ffissore commented May 5, 2018

I totally understand your problem and your focus on UX: please keep up the good work. I'll try to keep this in sync with master

@AndyWendt
Copy link

AndyWendt commented May 9, 2018

This seems to work for multiple request body examples but it doesn't seem to include changes for multiple response examples.

Otherwise, we totally want this and I probably could spare a member of my team to work on this if it would be helpful.

👍

@theangrydev
Copy link

@AndyWendt I have seen it working with multiple examples for both request and response bodies. I built a snapshot and have a live example here:
https://trafficparrot.com/documentation/3.13.x/openapi/index.html

Have a look at one of the POST sections.

@AndyWendt
Copy link

Interesting @theangrydev, thanks for the insight. I must be doing something wrong then.

@AndyWendt
Copy link

AndyWendt commented May 9, 2018

Yep, I was. Improper indentation in the yaml file

@theangrydev
Copy link

All hail yaml! King of the whitespace!

@codedmonkey
Copy link

The activeTab value should also get updated when switching between content types in case they have different examples. Currently it fails to load a tab at all when this is the case because the current activeTab value's tab doesn't exist.

@racci
Copy link

racci commented Jul 30, 2018

@theangrydev @AndyWendt
I downloaded .yaml file from following page and run it in swagger editor
https://trafficparrot.com/documentation/3.13.x/openapi/index.html

However, I still not able to see multiple body examples.
Am I missing something?

screen shot 2018-07-30 at 11 57 31 am

@theangrydev
Copy link

@racci The fix has probably still not been released yet. I compiled my own version of swagger ui with a fix that worked for me.

@racci
Copy link

racci commented Jul 30, 2018

Ohh I see. Thanks @theangrydev

Do you have any estimate when it may release?

@razerware
Copy link

razerware commented Aug 9, 2018

@theangrydev @racci i build my Swagger UI of V3.18 by Docker and I download .yaml from your snapshot. It seems mutli-examples still not work yet. I got same result as @racci.

@boesing
Copy link

boesing commented Sep 4, 2018

Well, since I am working with this Pull Request for about a few months, I can tell you guys that this is actually working as expected.
I would appreciate, if this is being merged to the next version. If there are bugs, those still can be fixed in one of the next releases.

What can we do to bring this up?

@Javivi
Copy link

Javivi commented Oct 3, 2018

Any progress towards a release?

@webron
Copy link
Contributor

webron commented Oct 10, 2018

We have an initial design to what it may end up looking like, but unfortunately no ETA yet.

@boesing
Copy link

boesing commented Jan 14, 2019

@ffissore As this worked for me for about a year now, I would like to see this PR rebased against the latest changes so I can update and just re-apply your changes on the latest code.

This probably makes life easier for @webron to merge this after they finally decided how they want to style this feature.
(At least thats what I understood what he meant in his last comment: its still a question about style.)

Sad story that this cannot be merged until the day a final design comes up. I dont see any issue with this and would really love to see this merged. There should be no issue to change the mockup in another release tho.

@ffissore
Copy link
Author

Apologies @boesing but in the meantime I've left the company so I don't have chances to rebase and test this PR

@boesing
Copy link

boesing commented Jan 17, 2019

@ffissore the code changes are in your account? dont understand why you are not able to do this but whatever. I dont see any way to get this even merged then. Hopefully some can take your ideas to apply them in a proper feature in one of the next releases.
thanks anyway.

@shockey
Copy link
Contributor

shockey commented Oct 3, 2019

Closing in favor of #5427. Thanks for the efforts here, @ffissore!

@shockey shockey closed this Oct 3, 2019
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.