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 js.erb response for prices#new and add breadcrumbs #1856

Merged
merged 2 commits into from
Apr 24, 2017

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Apr 22, 2017

The new link was previously using remote: true and a js.erb template which replaced the main body of the page with the new form. This PR changes it to link to the new url normally.

This fixes a minor bug where the NumberWithCurrency JS component wasn't being initialized on the dynamically added content. It also allows having the breadcrumbs and title change correctly.

If we wanted the benefits of the old behaviour (avoiding full page load), we should use turbolinks.

This was previously using remote: true and a js.erb template which
replaced the main body of the page with the new form. This just links to
the new url normally. If we wanted the behaviour this had, we should use
turbolinks.

This fixes a minor bug where the NumberWithCurrency JS component wasn't
being initialized on the dynamically added content. It also fixes the
breadcrumb heading.
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.

Good catch 👍

@tvdeyen
Copy link
Member

tvdeyen commented Apr 22, 2017

we should use turbolinks.

That's something I really want to look into next. Now that our JS is better organized this should be easy to accomplish.

@mtomov
Copy link
Contributor

mtomov commented Apr 23, 2017

we should use turbolinks.

That's something I really want to look into next. Now that our JS is better organized this should be easy to accomplish.

👍

I have been thinking the same those past couple of days, and even looked briefly at jquery.turbolinks

@tvdeyen
Copy link
Member

tvdeyen commented Apr 24, 2017

#1863

I have been thinking the same those past couple of days, and even looked briefly at jquery.turbolinks

jquery.turbolinks does not support Turbolinks 5 and is not needed by us. There might be an extension or store that uses an old jquery plugin that makes internal use of dom:ready, but this is most likely very rare.

Extensions and stores DO have to use Spree.ready in stead of jQuery.ready though. But this is a simple search and replace update.

@jhawthorn jhawthorn merged commit c73ea35 into solidusio:master Apr 24, 2017
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