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

Tidy templates for documentation views. #556

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

ngnpope
Copy link
Contributor

@ngnpope ngnpope commented Oct 7, 2021

For both Redoc & Swagger UI templates:

  • Make spacing in {{ ... }} tags consistent.
  • Make use of |default filter for default title.
  • No need for self-closing tags in HTML5.
  • Remove type="text/css" for <link rel="stylesheet" ...>
    • It's the implicit default since HTML5.

For Redoc template:

  • Remove unnecessary comments or convert to Django template comments.
  • Update to use newer method for loading fonts from Google.

For Swagger UI templates:

  • Don't force type="image/png" for Swagger favicon
    • The setting might point to another type, e.g. SVG.
  • Don't use the |safe filter unnecessarily, i.e. for script_url.
  • Various minor JavaScript tweaks, e.g. missing trailing semicolons, etc.
  • Align with styles from https://cdn.jsdelivr.net/npm/swagger-ui-dist@3.52.3/index.html

Split out from #544.

For both Redoc & Swagger UI templates:

- Make spacing in `{{ ... }}` tags consistent.
- Make use of `|default` filter for default title.
- No need for self-closing tags in HTML5.
- Remove `type="text/css"` for `<link rel="stylesheet" ...>`
  - It's the implicit default since HTML5.

For Redoc template:

- Remove unnecessary comments or convert to Django template comments.
- Update to use newer method for loading fonts from Google.

For Swagger UI templates:

- Don't force `type="image/png"` for Swagger favicon
  - The setting might point to another type, e.g. SVG.
- Don't use the `|safe` filter unnecessarily, i.e. for `script_url`.
- Various minor JavaScript tweaks, e.g. missing trailing semicolons, etc.
- Align with styles from https://cdn.jsdelivr.net/npm/swagger-ui-dist@3.52.3/index.html
@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #556 (5313f45) into master (2c91cbd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #556   +/-   ##
=======================================
  Coverage   98.76%   98.76%           
=======================================
  Files          58       58           
  Lines        6489     6489           
=======================================
  Hits         6409     6409           
  Misses         80       80           
Impacted Files Coverage Δ
tests/test_view.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c91cbd...5313f45. Read the comment docs.

@tfranzel
Copy link
Owner

tfranzel commented Oct 8, 2021

@ngnpope excellent changes. learnt some new things today. thx. 😄

the preconnect makes total sense. never used it before. I would have never noticed the swagger ui default style html there. The visual difference is tiny but it's there.

@tfranzel tfranzel merged commit 9f4e082 into tfranzel:master Oct 8, 2021
@ngnpope
Copy link
Contributor Author

ngnpope commented Oct 8, 2021

Thanks.

the preconnect makes total sense. never used it before.

I only noticed from using Google Fonts for something else and this is the output they're providing! A small modernisation.

I would have never noticed the swagger ui default style html there. The visual difference is tiny but it's there.

Yes - the background color was something I'd noticed in particular.

@ngnpope ngnpope deleted the improve-templates branch October 8, 2021 09:56
tfranzel added a commit that referenced this pull request Feb 10, 2022
django will escape & if used without filter and thus breaks url params
concat inside JS contexts. For HTML this is not an issue.

"safe" filter ignored this but is likely too broad, which was the reason
for the change. "escapejs" is specifically for this usecase.
@tfranzel
Copy link
Owner

@ngnpope fyi the removal of safe introduced a bug. the devil is in the details.... django does convert & to &amp; which breaks urls when inside JS context. see commit message for details.

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

2 participants