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

Replace static Swagger-ui distribution with unpkg version #1604

Merged
merged 12 commits into from
Jul 12, 2023

Conversation

theletterf
Copy link
Contributor

@theletterf theletterf commented Jul 5, 2023

This PR removes the static assets for swagger-ui and adds unpkg includes instead. I've also edited the docs accordingly.

@theletterf theletterf marked this pull request as draft July 5, 2023 13:12
@theletterf
Copy link
Contributor Author

@chalin @LisaFC Can't currently test the change. Could you have a look?

@LisaFC
Copy link
Collaborator

LisaFC commented Jul 5, 2023

Sure, happy to help, looks good from a quick glance but agree we'll need to check it works - is there a reason why you can't test it in your own project?

@theletterf
Copy link
Contributor Author

Currently don't have a local Docsy project running and got confused by docsy-example. What's the recommended way of testing features?

@LisaFC
Copy link
Collaborator

LisaFC commented Jul 5, 2023

Ah, gotcha - an easy way is to just look at the Netlify deploy preview as our docs use a huge range of Docsy features, but we don't have a Swagger UI page in the Docsy docs so the changes won't do anything in the site (other than confirm that they don't break anything else, that all seems fine....).

My usual slightly clunky approach (which is what I'll do if I need to test this :) ) is to clone/use a clone of the Docsy Example site, make the local overrides to the theme, add a Swagger UI page with the Swagger test "pet store" API, and then run it either locally or with Netlify and see whether the page shows up properly.

@chalin any thoughts?

@theletterf theletterf marked this pull request as ready for review July 5, 2023 14:30
@theletterf
Copy link
Contributor Author

theletterf commented Jul 5, 2023

Seems to be working fine!

To test this, I modified the following lines in the hugo.toml file of docsy-example:

# hugo module configuration

[module]
  # uncomment line below for temporary local development of module
  replacements = "github.com/theletterf/docsy -> ../../docsy"
  [module.hugoVersion]
    extended = true
    min = "0.110.0"
  [[module.imports]]
    path = "github.com/theletterf/docsy"
    disable = false
  [[module.imports]]
    path = "github.com/google/docsy/dependencies"
    disable = false

The only thing that bothers me is that the version number of the spec appears with white background, which makes it hard to read. Would it make sense to add some CSS?

@theletterf
Copy link
Contributor Author

@LisaFC Even though it's slightly out of scope, I've fixed the color issue and added a _swagger.scss file which can come in handy for customizing Swagger UI's look and feel a bit (or for hiding elements by using the display directive). WDYT?

@LisaFC
Copy link
Collaborator

LisaFC commented Jul 5, 2023

Oh that looks great! I see what you mean about the background contrast issue - @chalin what do you think?

@theletterf
Copy link
Contributor Author

@LisaFC Fixed that in 5f31dd2

Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Great, thanks! LGTM

@chalin
Copy link
Collaborator

chalin commented Jul 10, 2023

@LisaFC - can you give your stamp of approval too?

layouts/swagger/baseof.html Outdated Show resolved Hide resolved
@theletterf theletterf requested a review from deining July 11, 2023 07:43
Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

LGTM now.
Can you rebase your PR to HEAD of main, please?

@theletterf
Copy link
Contributor Author

@deining Done

@theletterf
Copy link
Contributor Author

Can't merge on my own!

@chalin
Copy link
Collaborator

chalin commented Jul 11, 2023

@LisaFC - still awaiting your approval here

Copy link
Collaborator

@LisaFC LisaFC left a comment

Choose a reason for hiding this comment

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

Looks fantastic - sorry for the late review.

@LisaFC LisaFC merged commit 92674f8 into google:main Jul 12, 2023
6 checks passed
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.

Docsy doesn't support rendering Swagger UI using OpenAPI 3.1.X swagger-ui DOM XSS
4 participants