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.

Sign up for GitHub

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

Jump to bottom

[RFC] Allow use of Turbolinks in admin #1863

Merged
merged 7 commits into from
Oct 4, 2017
Merged

[RFC] Allow use of Turbolinks in admin #1863

merged 7 commits into from
Oct 4, 2017

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Apr 24, 2017 •
edited
Loading

Using Turbolinks for navigating admin should give us a nice little speed gain.

For getting this to work it was needed to use Spree.ready instead of jQuery.ready to initialize the various JS modules we have.

Luckily Spree.ready already takes care of the turbolinks:load event, so this was easy for our own admin, for extensions we overwrote jQuery.ready to use Spree.ready instead (the same trick that jquery-turbolinks uses).

  • Disable Turbolinks per default and enable it only for new Stores
  • Override jQuery.ready and log deprecation notices to the JS console

@tvdeyen tvdeyen mentioned this pull request Apr 24, 2017
Merged
@tvdeyen tvdeyen added changelog:solidus_backend Changes to the solidus_backend gem type:enhancement Proposed or newly added feature SolidusConf Hack Day labels Apr 24, 2017
@tvdeyen tvdeyen requested a review from jhawthorn April 24, 2017 08:14
@mtomov
Copy link
Contributor

mtomov commented May 4, 2017

Super! Could you maybe push this code on a branch on the solidusio repo, as for some reason when I try to create a pull request towards your fork, I can't see it in the list ..

@tvdeyen
Copy link
Member Author

tvdeyen commented May 4, 2017

@mtomov sure https://github.com/solidusio/solidus/tree/turbolinks

This was referenced May 4, 2017
Merged
Merged
@mamhoff
Copy link
Contributor

mamhoff commented May 22, 2017

@mtomov is still working on this

@tvdeyen tvdeyen self-assigned this May 22, 2017
@tvdeyen tvdeyen removed the WIP label Jun 9, 2017
@tvdeyen tvdeyen removed their assignment Jun 9, 2017
@tvdeyen
Copy link
Member Author

tvdeyen commented Jun 9, 2017

This is now ready to review

@tvdeyen
Copy link
Member Author

tvdeyen commented Aug 11, 2017

@cbrunsdon rebased with latest master and ready to be reviewed.

@tvdeyen tvdeyen requested review from cbrunsdon and removed request for jhawthorn August 11, 2017 21:41
jhawthorn
jhawthorn reviewed Aug 30, 2017
View reviewed changes
backend/app/assets/javascripts/spree/backend/turbolinks-configuration.js.erb Outdated
@@ -0,0 +1,20 @@
// comment line necessary for correct interpolation of use_turbolinks
Turbolinks.supported = <%= Spree::Backend::Config.use_turbolinks %>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead would it be possible to do by having a user edit their vendor/assets/javascripts/spree/backend/all.js?

@tvdeyen tvdeyen force-pushed the turbolinks branch 2 times, most recently from a1961ce to dbb478e Compare September 6, 2017 15:24
@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 6, 2017

Like discussed in the last core team meeting we will ship everything that make Solidus admin compatible with Turbolinks, but do not enable it by default. Even not for newly created stores.

The chance to break something by an extension or usage of JS lib that is not compatible with Turbolinks exists and we do not want to maintain issues caused by usage of Turbolinks.

Use at your own risk. "It should work"™️

jhawthorn
jhawthorn approved these changes Sep 6, 2017
View reviewed changes
Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

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

👍

kennyadsl
kennyadsl approved these changes Sep 20, 2017
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

@tvdeyen tvdeyen force-pushed the turbolinks branch 2 times, most recently from cce3669 to 032589a Compare September 21, 2017 07:17
@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 21, 2017

@cbrunsdon rewrote the flaky spec.

tvdeyen and others added 7 commits October 4, 2017 14:38
@tvdeyen
Use Spree.ready to run DOM ready events
d53a3f8 
Instead of using jQuery.ready we now use Spree.ready that takes care of Turbolinks load event.
@tvdeyen
Use brandcolor for Turbolinks progress bar
9c08f76
@tvdeyen
Tell Turbolinks to track changes in admin assets
fab5f7c
@tvdeyen
Disable Turbolinks caching
5d17318 
Caching of Turbolinks visits causes more harm then benefits. For example we need to take care of tearing down all third party JS widgets (like select2, jQueryUI) on our own. Also lots of our JS is written in a way that it needs a fresh copy of the body to work with.
@mtomov @tvdeyen
Add Turbolinks integration file for admin area
686e2c1 
In order to enable Turbolinks in Solidus admin you need to load a fix
for extensions using JS libs that make use of jQuery.ready.

Add `gem 'turbolinks', '~> 5.0.0'` into your `Gemfile` (if not already present)
and append these lines to `vendor/assets/spree/backend/all.js`:

    //= require turbolinks
    //= require backend/app/assets/javascripts/spree/backend/turbolinks-integration.js
@tvdeyen
Removes redundant clicks from payment methods spec
a1c3ec8
@tvdeyen
Rewrite order with user update email feature spec
e4d201a 
This test was accessing the database inside a feature spec. Sometimes the request may not be finished yet before we ask the object for it's changed value.

Rewrote the spec so it tests what's actually changing on the page instead of in the database. This should stabilize the spec.
@tvdeyen
Copy link
Member Author

tvdeyen commented Oct 4, 2017

Rebased with latest master

@tvdeyen tvdeyen changed the title [RFC] Use Turbolinks in admin [RFC] Allow use of Turbolinks in admin Oct 4, 2017
gmacdougall
gmacdougall approved these changes Oct 4, 2017
View reviewed changes
Copy link
Member

@gmacdougall gmacdougall left a comment

Choose a reason for hiding this comment

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

👍 ☠️

@gmacdougall gmacdougall merged commit 2914c2d into solidusio:master Oct 4, 2017
@mtomov
Copy link
Contributor

mtomov commented Oct 5, 2017

That was a good decision. I like it better without the config option.

Thanks for all your work on that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmacdougall gmacdougall gmacdougall approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

@jhawthorn jhawthorn jhawthorn approved these changes

@cbrunsdon cbrunsdon Awaiting requested review from cbrunsdon

Assignees
No one assigned
Labels
changelog:solidus_backend Changes to the solidus_backend gem type:enhancement Proposed or newly added feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tvdeyen @mtomov @mamhoff @jhawthorn @kennyadsl @gmacdougall