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

Try-out button for Swagger file #1563

Merged
merged 15 commits into from
Sep 14, 2016
Merged

Try-out button for Swagger file #1563

merged 15 commits into from
Sep 14, 2016

Conversation

marla-singer
Copy link
Contributor

@marla-singer marla-singer commented Sep 12, 2016

Closes #1490

  • Add checkboxes on the Manage Documentation modal
  • Add autorun to render Swagger Ui after changing methods

@marla-singer
Copy link
Contributor Author

@preriasusi Could you please review?

@brylie
Copy link
Contributor

brylie commented Sep 13, 2016

This looks good, but is behaving very strangely.

E.g. After uploading a Swagger document, the documentation viewer renders, but none of the sub sections will open when clicking show/hide or the endpoint name.

I notice the following console errors:

modules.js:41892 Uncaught TypeError: $.param.fragment is not a function
modules.js:41929 Uncaught TypeError: Cannot read property 'pushState' of undefined

@brylie
Copy link
Contributor

brylie commented Sep 13, 2016

I am also noticing multiple sAlerts when clicking the 'choose file' button:

Documentation link successfully updated!

Check the event handlers for the template to see if they are overlapping.

@@ -34,6 +35,11 @@ Apis.schema = new SimpleSchema({
optional: true,
regEx: SimpleSchema.RegEx.Url
},
submit_methods: {
type: [String],
label: 'Allow try-out for following methods:',
Copy link
Contributor

Choose a reason for hiding this comment

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

@marla-singer
Copy link
Contributor Author

modules.js:41892 Uncaught TypeError: $.param.fragment is not a function

I suppose this error shows when npm packages installed/ran in the wrong order. If you delete folder 'node_modules' and reinstall packages it should work correctly

@brylie brylie added this to the Sprint 31 milestone Sep 13, 2016
@marla-singer
Copy link
Contributor Author

@brylie Done. I also update swagger-ui-browseify package version and I don't use window variable. But I will push it after merging #1555

@brylie
Copy link
Contributor

brylie commented Sep 14, 2016

@marla-singer is this ready for review? Please remember to toggle the 'ready for review' label, to make it clear when a PR is ready.

I will go ahead and review, but wanted to make sure the PR is ready.

@marla-singer
Copy link
Contributor Author

@brylie Yes, PR is ready

AutoForm.hooks({
apiDocumentationForm: {
onSuccess () {
sAlert.success(TAPi18n.__('manageApiDocumentationModal_LinkField_Updated_Message'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate this line into two parts:

// Get success message translation
const message = TAPi18n.__('manageApiDocumentationModal_LinkField_Updated_Message');

// Alert user of success
sAlert.success(message);

It is a bit 'cleaner' this way, and shortens the line length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@@ -16,7 +16,7 @@
<div id="message-bar" class="swagger-ui-wrap message-success" data-sw-translate=""></div>
{{#if dataFetched}}
{{#if documentationValid}}
{{>swaggerUiContent apiDocumentation=apiDocumentation}}
{{>swaggerUiContent apiDoc=apiDoc api=api}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Check the indentation on this file. It may be sufficient to use two space per nested level when indenting HTML files. This also matches our JS indentation convention.

@brylie
Copy link
Contributor

brylie commented Sep 14, 2016

I have done a review of the code. There are some 'lint/style' issues, but overall this is ready to merge soon.

@marla-singer
Copy link
Contributor Author

marla-singer commented Sep 14, 2016

@brylie Done. Thanks for your recommendations!

@brylie brylie merged commit 03fab3a into develop Sep 14, 2016
@brylie brylie deleted the feature/try-out-button branch September 14, 2016 10:07
@ilarimikkonen ilarimikkonen added this to In Review in apinf/platform Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants