-
Notifications
You must be signed in to change notification settings - Fork 141
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
version docs (fixes #121) #245
Conversation
f55bde7
to
dd36af0
Compare
@@ -30,7 +32,12 @@ And then link `aframe-site` to `aframe`: | |||
cd aframe-site | |||
npm link aframe | |||
|
|||
Then the documentation will update as you work on them from the A-Frame repository. This works because we have pointed the A-Frame site, via a soft symbolic link, to the documentation installed in `node_modules/aframe/docs/`. | |||
Then the `master` documentation will update as you work on them from the | |||
A-Frame repository. This works because we have pointed the A-Frame site, via a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anything else for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review it thoroughly tomorrow. thanks for this.
is there any way we can test with writing new docs for new versions but hide that version until it's ready and released? maybe some local dev flag. we should have one / be able to add one easily with hexo plugins (maybe browsersync). |
OK, I'll try |
Actually, the new docs are just |
^ So nothing else needed to be done so far. |
"generate": "hexo generate", | ||
"predeploy": "hexo clean", | ||
"deploy": "hexo deploy && open 'https://aframe.io/'", | ||
"pretest": "multidep multidep.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was not obvious to me that I had to run npm run pretest
before the symlinks would work. perhaps call this too w/ postinstall
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, i was following a guide that was doing this for testing. thanks for catching
can we fix those URL routes so we're not redirecting? because we don't have server-side redirects for any of these (yet, though I can manually add them all to our CloudFlare Page Rules), this is actually quite slow - even locally. |
<a href="<%- page_url(page.path) %>" class="guide-link guide-link-left"> | ||
← <%= page.title %> | ||
</a>< | ||
/span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking great! let me know when to take another look |
OK, hope I got everything.
|
@cvan ready |
At your earliest convenience! Somewhat antsy about this one since the documentation is wrong for 0.2.0 and people have been running into the same breaking changes and getting confused (e.g., |
no worries - testing now, will provide feedback momentarily |
var versions = aframeVersions.slice(0); | ||
var currentVersionIndex = versions.indexOf(currentVersion); | ||
versions.splice(currentVersionIndex, 1); | ||
return [currentVersion, MASTER].concat(versions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, changed.
previous reasoning was in order of relevance, but yeah it was weird.
5fb670d
to
10f5429
Compare
['docs/core/templates.html', 'https://github.com/ngokevin/aframe-template-component'], | ||
['docs/guide/cameras-and-lights.html', 'docs/guide/building-a-basic-scene.html'], | ||
['docs/guide/entering-vr.html', 'http://mozvr.com/#start'], | ||
['docs/guide/installation.html', 'docs/guide/getting-started.html'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting bug:
http://localhost:4000/docs/0.1.0/guide/installation.html can be loaded. it's now http://localhost:4000/docs/0.1.0/guide/getting-started.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand your point correctly, this redirect was before we were versioning. We wanted to redirect docs/guide/installation
(0.1.0) to docs/guide/getting-started.html
(0.2.0). Now that we have a version prefix, we don't necessarily have to redirect the 0.1.0/guide/installation.html
since that page exists now.
I think I will version the destination of these redirects though so that we're not constantly updating these on each release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I versioned the old redirects: https://github.com/aframevr/aframe-site/pull/245/files#diff-f38517d4dd3fc581f64643b3dd59e408R20
It's a bit confusing to think about, but I believe this makes sense, and users with old links should end up in the appropriate places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which of the following should docs/guide/installation.html
redirect to?
docs/guide/getting-started.html
docs/guide/0.1.0/installation.html
docs/guide/0.1.0/getting-started.html
docs/guide/0.2.0/installation.html
docs/guide/0.2.0/getting-started.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose docs/0.2.0/guide/getting-started.html
to keep things the way they are currently.
docs/0.1.0/guide/installation.html
is more accurate. But not sure about having redirected to 0.2.0
and then suddenly redirecting it back to 0.1.0
.
I just pulled, and http://localhost:4000/docs/ / http://localhost:4000/docs/0.2.0/guide/ no longer works |
^ Did you run the new Updated everything, last thing I need to do is remove unnecessary redirects by assigning a |
Actually, I can filter |
OK, all complete. Most recent commit has changes for deducing the first page of the docs. |
🐛 |
🐹 👀 |
looks like "Install" nav link is removed. IMO, we should have a very prominent "Install" or "Download" link: will file separately. filed #268 |
r+ this all looks great to me. 🎉 fantastic work, man! |
re: installed, Agreed. I don't think it was prominent where it was anyways. |
Thanks for the detailed review! |
Problem is we are currently only showing
master/
docs on aframe.io which gives people incorrect information as most users are on0.2.0
. Default to0.2.0
docs and add a dropdown to switch between them. The only con is not being able to upstream typo fixes in the0.2.0
docs since that would involve cherry-picking and too much effort. But at least we show the shinymaster/
versionmultidep
to install multiple versions of A-Frame.docs/0.2.0
symbolic links tomultidep/
.docs/master
symbolic links tonode_modules/
.docs/<docpath>
todocs/<VERSION>/<docpath>
. Include redirectsOther: