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.

Sign up for GitHub

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

Jump to bottom

Upgrade Swagger-UI to 3.22.0 #82

Merged
merged 1 commit into from
Mar 29, 2019
Merged

Upgrade Swagger-UI to 3.22.0 #82

merged 1 commit into from
Mar 29, 2019

Conversation

chenjr0719
Copy link
Member

Close #79

@hampsterx
Copy link
Contributor

cool, nothing busted in the project I checked out your branch on :)

That black header and logo sure is ugly tho!

https://stackoverflow.com/questions/45155881/swagger-ui-hide-definition-url-path

In Swagger UI 3.x, you can hide the top bar in one the following ways.

On another framework (not python) the swagger they output is this. I wonder if its better to just link to the cdn and optionally avoid the default theme, looks pretty clean without it!


<!DOCTYPE html>
<html>
<head>
    <meta charset="UTF-8">
    <script src="https://cdnjs.cloudflare.com/ajax/libs/swagger-ui/3.19.0/swagger-ui-bundle.js"></script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/swagger-ui/3.19.0/swagger-ui-standalone-preset.js"></script>
    <link rel="stylesheet" type="text/css" href="https://cdnjs.cloudflare.com/ajax/libs/swagger-ui/3.19.0/swagger-ui.css">

    <script>
 window.onload = function() {

        SwaggerUIBundle({
            url: "/swagger/swagger.yml",
          dom_id: '#swagger-ui'
        })
}

    </script>
</head>
<body>
    <div id="swagger-ui"></div>
</body>
</html>

@chenjr0719
Copy link
Member Author

@hampsterx IMHO, use CDN might cause issues if someone(me, for example) using sanic-openapi in the on-premises environment.
And the header, maybe we can provide some method/config(for example, app.config['SWAGGER_HIDE_HADER'] = True) to disable the header. What do you think?

@DirkGuijt
Copy link
Contributor

This is great! It also means that we can now separate the blueprints for OpenAPI and Swagger again. I joined these originally because of the issue with a relative path to the spec. But it makes more sense to keep them separate.

I'm also not a big fan of a link to the CDN and I'll have a look at the PR later today.

@DirkGuijt
Copy link
Contributor

If we want to change the color we can also just add some CSS code like this:

.swagger-ui .topbar { background-color: #89bf04; }

That will give it that old look again :)

@hampsterx
Copy link
Contributor

I looked at a handful of other openapi projects last night and most of them are inlining the html/assets and yes there is always someone who will want it offline :)

Using a swagger config as mentioned in #24 would allow it to be customized if need be.

@DirkGuijt
Copy link
Contributor

I feel like there is no big downside to provide an inline solution. It's just a few javascript files to host. If you have a problem with hosting this, then I suggest we split the OpenAPI spec and Swagger UI blueprints once again (now possible because of the relative URL fix) and let the users choose themselves if they want only the OpenAPI spec, or also the Swagger UI.

I think we are making it too complex by having the third option where you do have the OpenAPI spec and also host the HTML files but not the CSS and JS files. I don't understand why you would want this option.

@hampsterx
Copy link
Contributor

oh yup I have no problem with linline solution, seems common. I would merge this PR already but I don't have access. have asked @ahopkins about it.

@DirkGuijt
Copy link
Contributor

DirkGuijt commented Mar 28, 2019 •
edited
Loading

Do we want to do something out that ugly header color before we merge? 😛

Edit: I was talking about "you" in the general sense :) not you specifically

@hampsterx
Copy link
Contributor

not worried, its standard so whatever. would be keen to do a 0.5.1 release if we could get this and #22 and #80 in?

@chenjr0719
Copy link
Member Author

Personally, I would prefer a customizable solution rather than an inline solution. But, I think we don't have to make any decision now. Give it a little time and see do other users also having this requirement(as I know, #59 might request this feature).

@ahopkins ahopkins merged commit b202af2 into sanic-org:master Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirkGuijt DirkGuijt DirkGuijt approved these changes

@hampsterx hampsterx hampsterx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chenjr0719 @hampsterx @DirkGuijt @ahopkins