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

Fix embed urls : now complete (http and https wide) #49

Merged
merged 5 commits into from
Apr 3, 2014

Conversation

tibastral
Copy link
Contributor

No description provided.

@thibaudgg
Copy link
Owner

Specs? 🙏

@tibastral
Copy link
Contributor Author

Les specs passent :)

& BTW, j’aime beaucoup ton jeux de tests avec les Rémi Gaillard, etc.

Et aussi, je trouve ton travail super, et je t’en remercie du fond du coeur !

  • Thibaut
    freelancebooking.pro creator

On 3 Apr 2014 at 11:13:12, Thibaud Guillaume-Gentil (notifications@github.com) wrote:

Specs?


Reply to this email directly or view it on GitHub.

@karlentwistle
Copy link
Contributor

This PR is a great idea!

However in its current form. There doesn't seem to be any point in hanging onto

url_scheme: 'https

As in

expect(provider.embed_code(url_scheme: 'https')).to eq ......

@tibastral
Copy link
Contributor Author

Agreed

  • Thibaut
    freelancebooking.pro creator

On 3 Apr 2014 at 11:53:01, Thibaut Assus (thibaut@milesrock.com) wrote:

Les specs passent :)

& BTW, j’aime beaucoup ton jeux de tests avec les Rémi Gaillard, etc.

Et aussi, je trouve ton travail super, et je t’en remercie du fond du coeur !

  • Thibaut
    freelancebooking.pro creator

On 3 Apr 2014 at 11:13:12, Thibaud Guillaume-Gentil (notifications@github.com) wrote:

Specs?


Reply to this email directly or view it on GitHub.

@karlentwistle
Copy link
Contributor

I wonder if any current users would be relying on url_scheme returning a specific protocol other than the one they're already using.

For example

  • http website specifically opts for https
  • https website specifically opts for http

@tibastral
Copy link
Contributor Author

When you are on https, you want to have everything in https.

When you are on http, you don’t care about https

Look at that vimeo link to integrate urls : http://developer.vimeo.com/player/embedding

  • Thibaut
    freelancebooking.pro creator

On 3 Apr 2014 at 15:55:33, Karl Entwistle (notifications@github.com) wrote:

I wonder if any current users would be relying on url_scheme returning a specific protocol other than the one they're already using.

For example

http website specifically opts for https
https website specifically opts for http

Reply to this email directly or view it on GitHub.

@thibaudgg
Copy link
Owner

@karlentwistle I agree, good point.
@tibastral could you please clean every url_scheme logic? Thanks!

@tibastral
Copy link
Contributor Author

done

  • Thibaut
    freelancebooking.pro creator

On 3 Apr 2014 at 16:01:37, Thibaud Guillaume-Gentil (notifications@github.com) wrote:

@karlentwistle I agree, good point.
@tibastral could you please clean every url_scheme logic? Thanks!


Reply to this email directly or view it on GitHub.

thibaudgg added a commit that referenced this pull request Apr 3, 2014
Fix embed urls : now complete (http and https wide)
@thibaudgg thibaudgg merged commit 000611d into thibaudgg:master Apr 3, 2014
@thibaudgg
Copy link
Owner

Thanks all, 2.3.1 released!

@tibastral
Copy link
Contributor Author

genius :)

  • Thibaut
    freelancebooking.pro creator

On 3 Apr 2014 at 16:39:51, Thibaud Guillaume-Gentil (notifications@github.com) wrote:

Thanks all, 2.3.1 released!


Reply to this email directly or view it on GitHub.

@karlentwistle
Copy link
Contributor

Nice one tibastral 👍

@tibastral
Copy link
Contributor Author

<3

  • Thibaut
    freelancebooking.pro creator

On 3 Apr 2014 at 17:16:51, Karl Entwistle (notifications@github.com) wrote:

Nice one tibastral


Reply to this email directly or view it on GitHub.

@thibaudgg
Copy link
Owner

😍

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