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

leave asset_path functionality default #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tilsammans
Copy link
Contributor

won't force https into http anymore.
it would trigger a warning in the console.

won't **force** https into http anymore.
it would trigger a warning in the console.
@tf
Copy link
Member

tf commented Jun 21, 2017

EDIT: Commented into wrong PR.

Wondering why this was in there in the first place. What happens if I embed a chart via an https url but the Pageflow server does not support SSL for published entries? Is the default protocol then determined by the iframe src or by the parent document url?

@tilsammans
Copy link
Contributor Author

I didn't test to try, it could be interesting to know. I think embedding https on http is fine; it's the other way round that triggers warnings.

Given that Datawrapper now defaults to https we should think about these things. Oh and also they put protocol-relative URL in the embedcode, which could be puzzling for an editor. Not saying we should allow protocol-relative URLs, but it is a source of confusion.

@tf
Copy link
Member

tf commented Jul 10, 2017

This appears to be more complicated than I had expected: The erb template is evaluated during asset compilation. If an Rails.application.config.action_controller.asset_host is configured, it will be prepended and the asset_path calls might still result in http urls. As you mentioned, this is undesirable since it causes warnings with HTTPS. Can you try if using protocol: :relative results in the desired outcome?

I think, at the moment we do not accept https urls in the editor input, right? If Datawrapper provides https urls by default, that might well be worth a separate PR.

@tilsammans
Copy link
Contributor Author

I think, at the moment we do not accept https urls in the editor input, right? If Datawrapper provides https urls by default, that might well be worth a separate PR.

Absolutely, they provide a protocol-relative URI right now:

This is the direct link to your chart: //datawrapper.dwcdn.net/AzoJk/1/

@tilsammans
Copy link
Contributor Author

Hi Tim,

If we leave out the protocol, the helper will reuse the protocol in use. So https will stay https, and http will stay http. I think this small change is good to go, we can see these assets in Chrome again. :)

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.

2 participants