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

Display example value in Swagger ReadOnly documentation #4422

Merged
merged 11 commits into from
Jun 15, 2020

Conversation

dedece35
Copy link
Contributor

@dedece35 dedece35 commented Apr 7, 2018

Description

It's the continuity of my closed PR #3977 about example part.

SwaggerUI documentation in ReadOnly mode doesn't give example value.
This value is given when "Try it out" button is cliked.
Now, this example value is also displayed on readonly mode of swaggerUI.

Motivation and Context

I want to know example value for a type without clicking on "Try it out" button.

How Has This Been Tested?

To develop this feature, I followed CONTRIBUTING.md file such as :

  • I launched "npm run dev" to have the test page ("Pets" API)
  • I developed and verified results in page http://localhost:3200 and see the rendered result
  • I launched "npm test" and verified that all is OK

Screenshots (if appropriate)

No screenshot but "example value" is displayed like "available values" et "default value" after these two values

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@webron
Copy link
Contributor

webron commented Apr 8, 2018

This is the kind of PRs that benefit from a before and after screenshot. 😕

@webron
Copy link
Contributor

webron commented Apr 8, 2018

Not to say we don't appreciate the PRs! We do!

Just generally speaking, @shockey reviews the code, and I often review the functionality change. If it's visual, it takes me less time to review the PR because I don't necessarily have to run the app for the change, or even if I do, it's easier to spot the change.

@dedece35
Copy link
Contributor Author

dedece35 commented Apr 9, 2018

@webron I understand your remark.
Excuse me but I gave the wrong PR Url.
I didn't give a screenshot because I already did it in the previous PR #4191 (and not in #3977 )
You can see in it the screenshot with the "example attribute displayed under "default value".

Finally, the example value isn't displayed at all.
Maybe I should create an issue instead of this PR.
Excuse me for that.

Give me your preference ?
You accept this PR and this issue is fixed OR you refuse this PR and I create an issue and I push the fix (included in this PR) and push another PR ?

@webron
Copy link
Contributor

webron commented Apr 9, 2018

We're ok with getting PRs without opening issues beforehand, if they are described well. Sure, there's a risk that we won't accept the PR, but generally speaking it is acceptable.

I'm ok with accepting the PR's functionality as it is right now, just be aware that we're planning on improving that part in general and it may make this PR obsolete. However, since it gives an intermediate solution, there's value in it.

@shockey shockey requested review from webron and shockey April 9, 2018 22:57
@dedece35
Copy link
Contributor Author

@webron ok. thanks a lot.

Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

This PR needs tests!

End-to-end would be best. If you'd be hard-pressed to implement the tests, ping me and I can add some cases for you, and get this merged 😃

@dedece35
Copy link
Contributor Author

Hello @shockey ...
excuse me for the delay of my response.
To go forward, can you add one or two use cases as you mentionned it.
I will write others tests to finish this PR.

thanks a lot.

@dedece35
Copy link
Contributor Author

hi @shockey @webron
tests added.
I hope they are ok for you.
Tell me if ok or not.
I really need this feature for my company.

Thanks a lot.

@tim-lai tim-lai dismissed shockey’s stale review June 15, 2020 22:22

approval was conditional on implementing test (done)

@tim-lai tim-lai merged commit ca1b19a into swagger-api:master Jun 15, 2020
@tim-lai
Copy link
Contributor

tim-lai commented Jun 15, 2020

@dedece35 Merged! Thanks for the contribution and patience!

mattyb678 pushed a commit to mattyb678/swagger-ui that referenced this pull request Jun 24, 2020
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.

None yet

4 participants