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

Make all parameters that use file paths/URLs have consistent and correct logic #254

Closed
daattali opened this issue Sep 22, 2017 · 6 comments

Comments

@daattali
Copy link
Owner

There are several places in the codebase that accept a path/URL. For example: the links in the navbar (defined in the config file), the "bigimg" and "image" YAML parameters, the "avatar" config parameter, the "title-img" config param, the "navbar-img", "footer-img" and "page-img" config params (which are new thanks to you!), and maybe a few more that I can't remember right now.

The problem is that right now these parameters all work differently. Some of them only support local relative paths (such as "/img/foo.png") and some pinky support external URL. For local path, extra care needs to be taken to ensure that site.baseurl is always prepended, because otherwise it wouldn't work for GitHub Project Pages. For URLs, both http and https need to work. The different parameters support different combinations of these features, but they should all be fixed to work fully. I think that part of the problem is that each parameter uses its own logic for this, so it'd be a good idea to make a tiny "include-uri" template (as in, actually create a file under _includes/), make that template suppirt all 4 cases (local path that has an empty site.baseurl, local path that has a site.baseurl, http url, https url), and use that template for all the parameters.

Code samples:

All these instances need to be consolidated into one. This problem became apparent in #249

@sumitgpt
Copy link

sumitgpt commented Oct 4, 2017

Not an expert. But what if we just check if the image link like bigimg starts with "/". With Jekyll, you add images in assets and this folder is at the root level. So every local image will start with "/" so that image is looked up from the root. If the link starts with anything else we can consider it an external link.

Now coming to https & http. I've added https:// in attribute url in my config file. So all the absolute URL of my site are built with https. I'd want users to add the preferred version of their site's url in url attribute itself. This prevents unnecessary redirects. For an external URL, we can expect that user would add the URL with http or https in attributes like bigimg.

@daattali
Copy link
Owner Author

daattali commented Oct 4, 2017

I think what you describe in the first paragraph is a similar idea to what I have in the code in some places, it just needs to be abstracted/generalized better so that it can be used everywhere that uses URLs/paths. I would prefer against requiring users to set a url config, all these things should work with minimal user intervention

@OCram85
Copy link
Contributor

OCram85 commented Nov 6, 2017

Finally I found some time working on this. I started with splitting down the input in its parts:

{%- assign protocol = include.link | split: "://" | first -%}
{%- assign full_path = include.link | split: "://" | last -%}
{%- assign domain = full_path | split: "/" | first -%}
{%- assign file = full_path | split: "/" | last -%}
{%- assign internal_base = protocol | append: "://" | append: domain -%}

{%- if protocol contains "http" -%}
{%- else -%}
    {%- assign protocol = false -%}
{%- endif -%}

Protocol: {{ protocol }}
Full Path: {{ full_path }}
Domain: {{ domain }}
File: {{ file }}
Internal Base = {{ internal_base }}

2017-11-06 15_27_42-get-uri test

But my question now is: What's the desired output format?

If I got it right I would go on with this:

  • If it's a internal link always add BasePath and transform to absolut path? - Or do you wand relative paths for internal links?
  • If it's a external link check the protocol prefix and the format in general?

@daattali
Copy link
Owner Author

daattali commented Nov 6, 2017

Do these also take care of site.baseurl?

Is it correct that the last example you have has :// after the image path?

@OCram85
Copy link
Contributor

OCram85 commented Nov 6, 2017

Ohh this is just a work in progress example. It should show my current approch how to split a URI into its parts. But I wasn't sure how the composed output should look like.

@daattali
Copy link
Owner Author

daattali commented Jan 5, 2018

@OCram85 feel free to continue the work on this if you find some free time. Your other contributions are much appreciated!

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

No branches or pull requests

3 participants