Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Add baseUrl option #65

Merged
merged 6 commits into from
Apr 9, 2020
Merged

Add baseUrl option #65

merged 6 commits into from
Apr 9, 2020

Conversation

kean
Copy link
Contributor

@kean kean commented Apr 9, 2020

Fixes #56

  • Add a new --base-url (baseURL: String) option
  • Update HTML generation to use the new baseUrl option
  • Update generated links to be relative

Usage:

swift doc generate ~/Develop/Nuke/Sources/ \
    --output Documentation \
    --format html \
    --module-name Nuke \
    --base-url https://kean-org.github.io/docs/nuke-docs-beta/9.0.0/

Notes

I made the minimum number of changes needed to make https://kean-org.github.io/docs/nuke-docs-beta/9.0.0/ work. If you don't think this is the right direction, feel free to close it. I'll keep my workaround locally for now.

What I'm not happy about in this solution is that it is not going to work if you browse it locally. Jazzy uses relative URLs in a form of <a href="../ImageCaching">. There are two major advantages. It works regardless of how you deploy it (root or not). And it works locally.

@mattt
Copy link
Contributor

mattt commented Apr 9, 2020

Great work, @kean! Thanks so much for taking this on.

I pushed a few (small) changes to your PR branch, all to do with naming and documentation. Please take a look at those and let me know if you have any objections to them.

What I'm not happy about in this solution is that it is not going to work if you browse it locally. Jazzy uses relative URLs in a form of <a href="../ImageCaching">.

Browsing locally — that is opening file:/// URLs in Safari — has a lot of caveats. Safari imposes various restrictions on what resources can be loaded / what scripts can be executed for local HTML files (see "Develop > Disable Local File Restrictions"), while macOS itself has ratcheted up security in recent years to require apps to get permissions for accessing local directories. And that's just one browser / OS combination — (macOS / iOS / Linux / Windows) × (Safari / Chrome / Firefox / Edge)... the combinatorics become overwhelming.

As we increase the capabilities of the HTML output, it'll be difficult enough to guarantee common behavior for normal web standards stuff. I'd hate to layer on the additional complexity of handling file:/// URLs when you can easily start a local web server.

(That said, we can certainly do more to document how to do this. And, actually, I think this is a perfect opportunity for us to dogfood a "Guide"-style document as discussed in #57. Not a blocker for this, but something to consider as a next step!)

Copy link
Contributor

@mattt mattt left a comment

Choose a reason for hiding this comment

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

Once again, great work on this, @kean! The only other change I'd like to request is for you to add an entry to the Changelog. As soon as that's in, we can merge this in 🎉

@kean
Copy link
Contributor Author

kean commented Apr 9, 2020

Thanks, @mattt, I agree with your changes. Updated changelog.

As we increase the capabilities of the HTML output, it'll be difficult enough to guarantee common behavior for normal web standards stuff.

You make a good point, I agree with you. It's not going to be feasibly to keep everything working locally when more features are added. And it's not worthwhile, nobody browses these files locally, probably except me.

Changelog.md Outdated
Comment on lines 8 to 10
## [Unreleased]

## TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## [Unreleased]
## TBD
## [Unreleased]

@mattt
Copy link
Contributor

mattt commented Apr 9, 2020

Thanks for adding that Changelog entry. Just a couple formatting fixes (I was hoping to commit from the suggestions, but that wasn't working for the header deletion)

@kean
Copy link
Contributor Author

kean commented Apr 9, 2020

No worries, I pushed the updated changelog.

@mattt mattt merged commit f3e2185 into SwiftDocOrg:master Apr 9, 2020
@mattt
Copy link
Contributor

mattt commented Apr 9, 2020

Alright, merged! Thanks again for your help, @kean 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Path-relative URLs are not working when deployed to GitHub Pages
2 participants