Skip to content

Docs: index.html refactored #11078

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

Merged
merged 6 commits into from
Apr 1, 2017
Merged

Docs: index.html refactored #11078

merged 6 commits into from
Apr 1, 2017

Conversation

jostschmithals
Copy link
Contributor

@jostschmithals jostschmithals commented Mar 28, 2017

This PR with a complete refactoring of the doc’s index.html (and some additional changes in the associated files) was done to handle the following issues:

  • Visitors coming from google are trapped on the subpages (links not working, no navigation panel) Docs: Visitors trapped on the subpages. – Server-side redirect possible?  #10937
  • The path structure of the URL fragments used by index.html for the iframes is different from the original path structure of the subpages (for example ... docs/api/renderers/webgl/plugins/LensFlarePlugin.html
    vs. … docs/#Developer_Reference/WebGLRenderer.Plugins/LensFlarePlugin Docs: Redirects for subpages implemented #10948 (comment)
  • Browser navigation (back/forward arrows) is not working Docs: 1. Browser navigation not working; 2. Query strings needed? #11052
  • Right click on a link in the navigation panel displays in the new tab only the navigation panel with a blank content area, instead of the correct content
  • The status bar displays https://threejs.org.docs/#, when clicking a link in the navigation panel (which is useless), and it displays javascript:window.parent.goTo(‘…’), when clicking on a link in the content area - which is irritating especially in those cases where instead of moving to another content only a tooltip with the basic JavaScript type (“Float”, “Array”) is displayed
  • Some other (minor) issues

(List edited after review)

On this occasion I tried to make the whole code structure more transparent and to improve the code style, too (see #11073).

Especially Edge was behaving differently in several cases. So there are some things in the code a little more inconvenient than it actually should have been. I hope, this is now working in all browsers (not only on Windows?).

Please test it and find the bugs ;-)


Links to subpages without navigation like
docs/api/core/Geometry.html
are now displayed as iframes of the index page, the URL fragment reflecting the original path structure:
... docs/#api/core/Geometry

Links to anchors in subpages like
docs/api/core/Geometry.html#morphTargets
are now displayed as
... docs/#api/core/Geometry.morphTargets
since '#' is not allowed inside an URL fragment (according to RFC 3986)

@looeee
Copy link
Collaborator

looeee commented Mar 28, 2017

Live version for testing

https://rawgit.com/jostschmithals/three.js/docsIndexRefactoring/docs/

@looeee
Copy link
Collaborator

looeee commented Mar 28, 2017

Won't this break existing links?

@mrdoob
Copy link
Owner

mrdoob commented Mar 28, 2017

Won't this break existing links?

Yeah, better sooner than later though...

@jostschmithals
Copy link
Contributor Author

jostschmithals commented Mar 28, 2017

@looeee Of course you are right – if we remove one of the two concurrent path structures, the one or the other will no longer work (And each kind of automatic forwarding would result in having the two systems forever.).

Therefore all links using the old ‘#’-system should now lead to the index page, showing the old fashioned URL in the address bar and a 404 message in the iframe. This is not very nice, but also not as bad, since the navigation panel should work, so that from there you can move where you want.

@mrdoob But perhaps we should take the existing old links into consideration by implementing a check if the requested iframe exists (doing an XMLHttpRequest before), so that we can display a friendlier message, saying that the requested page has moved and can easily be found by using the filter input field (Disadvantage though: we would always have an additional XMLHttpRequest)?

docs/page.js Outdated

}

window.location.replace( window.location.origin + '/docs/#' + window.location.pathname.slice(6, -5) + hash );
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this is hardcoded for https://threejs.org/docs/...?
Any way this could be more dynamic so even this link would work?
https://rawgit.com/jostschmithals/three.js/docsIndexRefactoring/docs/manual/introduction/Creating-a-scene.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will give this a try once more.

This had been a change from my concept, since without hardcoding it didn’t work in Edge.

Copy link
Owner

@mrdoob mrdoob Mar 28, 2017

Choose a reason for hiding this comment

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

I think you should be ble to do window.location.pathname.lastIndexOf('/docs/').

docs/index.html Outdated
var pageURL = pages[ pageName ];

var listElement = createAndAppendDOMElement( { type: 'li', parent: categoryContent } );
var linkElement = createAndAppendDOMElement( { type: 'span', parent: listElement, content: pageName } );
Copy link

Choose a reason for hiding this comment

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

Instead of a <span>, should this be an <a> with the href set to pageUrl? That way you can open these links in new tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current docs <a href=’#’> is used in this place. I only replaced it by a span to avoid the useless message in the status bar (as mentioned above). Of course this is not as important, and it could be reverted from <span> to <a>.

But I never used the new-tab-possibility in the current docs, since with right click you only get a new main window with a blank iframe, and with Ctrl+click the iframe reloads in the same tab.
.
Simply writing <a href = pageUrl> would not work anyway, since in this way the new source would not open in the iframe but on the top level. The situation is more complex here, and even a little more complicated than in the current version of the docs, since creating a working browser navigation together with iframes is a little tricky (see my comments in the code).

Copy link

@jaxry jaxry Mar 28, 2017

Choose a reason for hiding this comment

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

I don't exactly understand what the issue is. After creating the linkElement I can add

linkElement.setAttribute( 'href', '#' + pageURL );

to the code and everything still appears to work. In the event listener, you're preventing default behavior, so clicking the <a> tag won't function any differently than clicking a <span> tag. The only difference is that you can now right click the element and open the link in a new tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaxry Thank you for insisting ;) - you are completely right. Seems that I was too much influenced by the current version of the docs where this doesn’t work correctly.

Had not much time this evening, and now it’s too late. Will make a new PR with your improvement and with a "non-hardcoded" redirection tomorrow (similar to what @mrdoob suggested; this is working, too).

Copy link
Owner

Choose a reason for hiding this comment

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

No need to do new PRs by the way, you can just push new commits to the branch of this PR.

@jostschmithals
Copy link
Contributor Author

jostschmithals commented Mar 29, 2017

I made 2 new commits to keep the things separately. Both seem to function.

No need to do new PRs by the way

@mrdoob Thank you for the hint (but "PR" was only a "slip of the pen" ;) )


text = text.replace( /\[(?:member|property|method):([\w]+)\]/gi, "[member:$1 $1]" ); // [member:name] to [member:name title]
text = text.replace( /\[(?:member|property|method):([\w]+) ([\w\.\s]+)\]/gi, "<a href=\"javascript:window.parent.goTo('" + name + ".$2')\" target=\"_parent\" title=\"" + name + ".$2\" class=\"permalink\">#</a> .<a href=\"javascript:window.parent.goTo('$1')\" title=\"$1\" id=\"$2\">$2</a> " );
text = text.replace( /\[(?:member|property|method):([\w]+) ([\w\.\s]+)\]/gi, "<a onclick=\"window.parent.setUrlFragment('" + name + ".$2')\" target=\"_parent\" title=\"" + name + ".$2\" class=\"permalink\">#</a> .<a onclick=\"window.parent.setUrlFragment('$1')\" title=\"$1\" id=\"$2\">$2</a> " );
Copy link

@jaxry jaxry Mar 30, 2017

Choose a reason for hiding this comment

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

Is it possible to give these <a> tags working hrefs as well? For each page name (such as Matrix4 or Object3D), you'd need the corresponding section and category names (api/math or api/core respectively) to create a full url.

To retrieve the section and category names, I believe you could create a function in the parent (inside index.html) which returns the full url fragment given a page name. You'd call it from page.js and set the return value on the href.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment I can’t imagine an easy way of doing this without breaking the current functionality again (browser navigation in spite of iframes, links from outside displaying navigation panel plus proper content and so on).

But see my general answer to your further post.

@jaxry
Copy link

jaxry commented Mar 30, 2017

Is an iframe really necessary in the first place? It introduces a lot of extra complication having to manage the parent and child windows separately despite the fact the content is interconnected.

I'm wondering if it'd be simpler to load a selected html file with an XMLHttpRequest and to populate a div with the content once you've processed the text.

@jostschmithals
Copy link
Contributor Author

My answer is two-part:

On the one hand I regard this PR only as a patch that fixes some bugs (concerning non-working links and missing navigation panel when entering subpages from google, historical grown different path structures, non-working browser navigation, and so on), so I made no fundamental changes that would have touched the underlying iframe structure.

In my opinion this patch contains some urgent fixes which should be in the next release.

On the other hand my vision for the long term would be to make a completely new SPA (using AJAX instead of iframes, since all contents are coming from the same domain), reconsidering not only the underlying technique but also aspects of functionality/user experience.

This should be technically simpler indeed, but making bigger changes would require more and longer discussion, particularly in view of the fact that an all-in-one solution with the examples page and the three.js main page would be preferable.

So to my mind bigger changes (if @mrdoob wants them) should be made as a second step and without any time pressure.

@mrdoob
Copy link
Owner

mrdoob commented Mar 30, 2017

Do you mind using tabs for indentation? 😇

@jostschmithals
Copy link
Contributor Author

jostschmithals commented Mar 31, 2017

Sorry – I had relied on the validation by mrdoobapproves, not taking into account that this tool can’t check correct indentation style.

An additional question I forgot before:
In the end of the original index.html is the line <script src="../build/three.min.js"></script> <!-- console sandbox -->. Is it okay that I deleted this or is this actually used in the docs for something I don’t know?

@mrdoob
Copy link
Owner

mrdoob commented Mar 31, 2017

An additional question I forgot before:
In the end of the original index.html is the line <script src="../build/three.min.js"></script> <!-- console sandbox -->. Is it okay that I deleted this or is this actually used in the docs for something I don’t know?

This was added because some people liked to open the console while in the docs and test the API. I would leave it.

Everything else looks perfect though!

@jostschmithals
Copy link
Contributor Author

Done :)

@mrdoob
Copy link
Owner

mrdoob commented Apr 1, 2017

Sweet!

@mrdoob mrdoob merged commit 7e0d2b7 into mrdoob:dev Apr 1, 2017
@mrdoob
Copy link
Owner

mrdoob commented Apr 1, 2017

Thanks!

@jostschmithals jostschmithals deleted the docsIndexRefactoring branch April 27, 2017 14:20
@mrdoob
Copy link
Owner

mrdoob commented Oct 14, 2017

@jostschmithals Any chance you can take a look at this?
https://twitter.com/_gaeel_/status/918848846268157953

@looeee
Copy link
Collaborator

looeee commented Oct 14, 2017

Yeah, the lack of middle click navigation for internal links is kind of annoying.

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.

4 participants