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

Detect absolute URL’s in the extra_* path. #92

Merged
merged 8 commits into from Aug 18, 2014

Conversation

@ericholscher
Copy link
Contributor

@ericholscher ericholscher commented May 29, 2014

refs #91

<link href="{{ base_url }}/{{ path }}" rel="stylesheet">
{% if '//' in path %}
<link href="{{ path }}" rel="stylesheet">
{% elif 'media' in path %}

This comment has been minimized.

@tomchristie

tomchristie May 30, 2014
Contributor

Is this intended to simply be {% else %}? What's the 'media' in path check for?

This comment has been minimized.

@ericholscher

ericholscher May 30, 2014
Author Contributor

Yea, sorry. That was cruft left over from testing locally.

This comment has been minimized.

@jimporter

jimporter May 30, 2014
Contributor

It'd be nice if this wasn't done in the HTML templates; doing it this way means every theme needs to handle this if we want them to have feature-parity.

This comment has been minimized.

@ericholscher

ericholscher May 30, 2014
Author Contributor

Agreed. I stuck it there because we are doing the base_url logic there as
well. I agree we could pre-process the URL's in the list and just include
them in the template as a single variable.

On Fri, May 30, 2014 at 11:23 AM, Jim Porter notifications@github.com
wrote:

In mkdocs/themes/readthedocs/base.html:

@@ -11,13 +11,20 @@

{% for path in extra_css %} - - {% if '//' in path %} - - {% elif 'media' in path %}

It'd be nice if this wasn't done in the HTML templates; doing it this way
means every theme needs to handle this if we want them to have
feature-parity.


Reply to this email directly or view it on GitHub
https://github.com/tomchristie/mkdocs/pull/92/files#r13242790.

Eric Holscher
Maker of the internet residing in Portland, Or
http://ericholscher.com

@ericholscher
Copy link
Contributor Author

@ericholscher ericholscher commented May 30, 2014

Went ahead and added this as a util function. Not sure if it fits in with the code-base or style perfectly. It seems like we should probably be using the urlparse module for this, but every time I've worked with it I want to stab something :)

I'm not sure if we want to support protocol-less things (eg. "media.cdn.org/foo.js") -- this code is naive and will include that as a local URL.

@hdngr
Copy link

@hdngr hdngr commented Jun 13, 2014

would love to see Mkdocs support for Readthedocs...

@ericholscher
Copy link
Contributor Author

@ericholscher ericholscher commented Aug 15, 2014

I believe this should be merge-able now, if anyone would care to take a look.

try:
from urlparse import urlparse
except ImportError:
from urllib.parse import urlparse

This comment has been minimized.

@d0ugal

d0ugal Aug 15, 2014
Member

Can you import urlparse from mkdocs.compat?

This comment has been minimized.

@tomchristie

tomchristie Aug 15, 2014
Contributor

Yup, we should always keep version-branching code in compat.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Aug 15, 2014

I'm not even sure it's worth waiting for that tiny nitpick, but it would be good to remain consitent.

I'll wait and see if you happen to have time to update, otherwise I think this is good to merge.

@ericholscher
Copy link
Contributor Author

@ericholscher ericholscher commented Aug 16, 2014

Updated

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Aug 16, 2014

Thanks! Looks like a linting error is breaking the Travis run tho' :(

@ericholscher
Copy link
Contributor Author

@ericholscher ericholscher commented Aug 17, 2014

Last try? :)

@edbrannin
Copy link
Contributor

@edbrannin edbrannin commented Aug 17, 2014

Shouldn't this be updating mkdocs/themes/mkdocs/base.html as well?

According to grep, mkdocs and readthedocs are the only themes that support extra_*:

(env)Ed-Brannins-MacBook-Pro:mkdocs-prs ed$ ack extra_  mkdocs/themes/
mkdocs/themes/mkdocs/base.html
19:        {% for path in extra_css %}<link href="{{ base_url }}/{{ path }}" rel="stylesheet">{% endfor %}
63:        {% for path in extra_javascript %}<script src="{{ base_url }}/{{ path }}"></script>{% endfor %}

mkdocs/themes/readthedocs/base.html
13:  {% for path in extra_css %}
19:  {% for path in extra_javascript %}
@ericholscher
Copy link
Contributor Author

@ericholscher ericholscher commented Aug 18, 2014

Thanks @edbrannin -- updated the default theme too :)

@tomchristie
Copy link
Contributor

@tomchristie tomchristie commented Aug 18, 2014

Rad.

tomchristie added a commit that referenced this pull request Aug 18, 2014
Detect absolute URL’s in the extra_* path.
@tomchristie tomchristie merged commit bd55377 into mkdocs:master Aug 18, 2014
1 check passed
1 check passed
@tomchristie
continuous-integration/travis-ci The Travis CI build passed
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
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants