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

Remove use of require_tree in admin javascripts #1613

Merged
merged 4 commits into from
Nov 30, 2016

Conversation

jhawthorn
Copy link
Contributor

This came out of discussion with @tvdeyen

When we use require_tree or a relative path to require, it makes it so that attempting to override these files in your own app won't work.

This PR replaces all require_relative calls with an individual hardcoded require per-file. Requires for templates were split into their own index.js file

Fixes #1567

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

This is great. Thanks for doing the actual hard work, John!

One small thing left, though

//= require spree/backend/address_states
//= require spree/backend/adjustments
//= require spree/backend/admin
//= require spree/backend/backbone-overrides
Copy link
Member

Choose a reason for hiding this comment

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

You already required this in L15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I think in the future we could handle all folders like the spree/backend/templates folder, but this is great for now and fixes #1567

@tvdeyen
Copy link
Member

tvdeyen commented Nov 24, 2016

Using this fix in my demo app https://github.com/tvdeyen/solidus-asset-override-demo works 👍

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

Yea, this has tripped people up for an incredibly long time, good by me thanks.

@tvdeyen
Copy link
Member

tvdeyen commented Nov 25, 2016 via email

@jhawthorn jhawthorn merged commit 0ba3f0d into solidusio:master Nov 30, 2016
@jhawthorn jhawthorn deleted the remove_require_tree branch November 30, 2016 23:12
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

3 participants