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

Add base_path option to include asset methods #7

Closed
wants to merge 2 commits into from

Conversation

jboler
Copy link
Contributor

@jboler jboler commented Dec 30, 2015

Ignore previous comments - I got the specs working after looking at the travis config.

inject(&:+)
end

def include_ember_stylesheet_tags(name)
def include_ember_stylesheet_tags(name, base_path=nil)

Choose a reason for hiding this comment

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

Surrounding space missing in default value assignment.

@@ -15,6 +15,12 @@
expect(page).to have_javascript_rendered_text
end

scenario "rendering with asset helpers and absolute base_path from `/relative/`" do

Choose a reason for hiding this comment

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

Line is too long. [85/80]

@jboler
Copy link
Contributor Author

jboler commented Jan 11, 2016

@seanpdoyle Is there any chance this could be merged?

@franzliedke
Copy link

Yes, please! I wish it would do this automatically, but at least this gives me a way to fix the base URL problem. :)

@seanpdoyle
Copy link
Owner

@jboler thanks for opening this pull request -- sorry for the delay!

I'm sorry, but I don't understand the current implementation doesn't work for you -- would you mind explaining how it doesn't support your use case?

cc: @franzliedke

@franzliedke
Copy link

It's a path relative to the current URL, not the site's base URL.

@seanpdoyle
Copy link
Owner

@franzliedke I understand the new functionality, but I don't understand the use case.

Given the following situations:

  • viewing a "/" page, with assets served from your server, browsers will resolve vendor.js to /vendor.js
  • viewing a "/child" page, with assets served from your server, browsers will resolve vendor.js to /child/vendor.js
  • viewing a "/child" page, with assets served from a CDN, ember-cli-build.js configured to prepend the CDN host, and a <base href="/child"> element, browsers will resolve vendor.js to https://cdn.example.com/child/vendor.js

In what ways do your use cases not fit into one of those scenarios?

@jboler
Copy link
Contributor Author

jboler commented Jan 15, 2016

See my issue: #6

@franzliedke
Copy link

Huh? How does that help me when I render the ember_script_tags both on /foo/ and on /foo/bar/?

@seanpdoyle
Copy link
Owner

@jboler thanks for the clarification -- I understand now.

Instead of extending this gem to further monkey with the EmberCLI-generated URLs, would you be able to achieve the same thing by prepending asset URLs with the absolute path during EmberCLI's asset compilation?

@seanpdoyle
Copy link
Owner

@franzliedke thanks for mentioning that you're rendering script tags on both /foo/ and /foo/bar/.

Could you please elaborate further on how you're trying to use the gam, and the ways in which the current implementation falls short so that I can better understand your problem and possibly offer an alternative solution?

@jboler
Copy link
Contributor Author

jboler commented Jan 15, 2016

@seanpdoyle I can't do that during asset compilation because (as noted in the issue) the <base> tag breaks SVG <def> paths & <a> anchor paths. Basically I think the <base> tag is being used incorrectly with ember-cli-rails-assets because it is applied to every URL on the page when there are lots of valid use cases where one doesn't want this to happen.

Also, my Rails app has many different URLs where I need the same Ember app to load so I need include_ember_script_tags() to be able produce an absolute URL.

@seanpdoyle
Copy link
Owner

Merged in 23f9425.

Thanks!

@seanpdoyle seanpdoyle closed this Jan 16, 2016
@skylar
Copy link

skylar commented Mar 17, 2016

@seanpdoyle We had poltergeist/capybara-based test that were failing using the <base> tag method, and it was holding up our migration to ember-cli 2.3 and your awesome updates on ember-cli-rails. As I was starting to write the same patch, we found and pulled this PR which made the build green.

Looking forward to a new gem version published with this fix. Thanks for supporting this project for the community!

@seanpdoyle
Copy link
Owner

@skylar @jboler this is part of the 0.6.2 release.

@skylar
Copy link

skylar commented Mar 18, 2016

@seanpdoyle thanks! 👍

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.

None yet

5 participants