Configure admin turbolinks #1882

mtomov · 2017-05-04T10:13:54Z

Enabled for new applications

Disabled for existing applications. Enable in an initializer with:

# config/initializers/spree.rb
Spree::Backend::Config.configure do |config|
  config.use_turbolinks = true
end

Handles gracefully existing javascript code, which still uses
jQuery.ready and gives a warning of the location of those functions

Towards #1863

edit: I will see to fix the 18 test failures .. tests fixed!

houndci-bot · 2017-05-04T10:14:03Z

backend/app/models/spree/backend_configuration.rb

+    # @!attribute [rw] use_turbolinks
+    preference :use_turbolinks, :boolean, default: false
+
+


Extra blank line detected.

Please satisfy Hound here.

tvdeyen

Nice work 👌

Especially the fallback to Spree.ready is a neat trick that will help stores and extensions to upgrade!

I would like to have some small changes and please send the last commit as PR to current master as I want to see them pass without Turbolinks first.

Thanks for your work on this 👏

tvdeyen · 2017-05-09T07:42:11Z

backend/app/assets/javascripts/spree/backend/admin.js

@@ -159,7 +159,6 @@ Spree.ready(function(){
      return ui;
  };

-  $('table.sortable').ready(function(){


This is a change in the actual implementation and not necessary for Turbolinks to work, right? I think this should not be in this PR.

tvdeyen · 2017-05-09T07:45:21Z

backend/app/assets/javascripts/spree/backend/turbolinks-configuration.js.erb

+// comment line necessary for correct interpolation of use_turbolinks
+Turbolinks.supported = <%= Spree::Backend::Config.use_turbolinks %>;
+
+// override jQuery.ready to support Turbolinks


This comment is misleading. You are doing this because we want to print out a deprecation warning and use Spree.ready even if it was not used explicitly. Turbolinks works without this change.

tvdeyen · 2017-05-09T07:47:32Z

backend/app/models/spree/backend_configuration.rb

+    # @!attribute [rw] use_turbolinks
+    preference :use_turbolinks, :boolean, default: false
+
+


Please satisfy Hound here.

tvdeyen · 2017-05-09T07:50:55Z

backend/app/assets/javascripts/spree/backend/turbolinks-configuration.js.erb

@@ -0,0 +1,20 @@
+// comment line necessary for correct interpolation of use_turbolinks
+Turbolinks.supported = <%= Spree::Backend::Config.use_turbolinks %>;


Neat little trick 👌

But we remove the internal check of Turbolinks for supported browsers with this. Imagine someone enables Turbolinks in the backend and uses an unsupported browser, Turbolinks will not gracefully fall back anymore.

I can live with that, because we encourage admin users to use latest browsers, but this can cause issues.

Thanks for the feedback! I will sort out the rest if we get the #1900 merged in.

On this one, we can copy over the few lines from turbolinks, but I really hate to encourage people to use old browsers, and those unsupported browsers are already ice age by now, let alone by the time somebody actually starts using this feature here..

So, well pointed out; my preference is to keep this behaviour.

Hi @tvdeyen , sorry to pester .. just whenever you get to it .. if you could rebase your branch on top of master now that that preparation PR has been merged .. as otherwise I think I won't be able to get the tests to green?

@mtomov sure. Sorry, I forgot the target of this PR, you are totally right!

No problem, but I think that you might have updated tvdeyen/turbolinks instead of solidusio/turbolinks : )

@mtomov updated solidusio/turbolinks

tvdeyen · 2017-05-09T07:55:15Z

backend/spec/features/admin/orders/new_order_spec.rb

@@ -175,8 +170,6 @@
    end

    it "transitions to delivery not to complete" do
-      click_on 'Cart'
-


Can we have these changes to the new order and customer details feature specs as separate PRs to master? I want to be sure they still pass on non-Turbolinks based backend.

tvdeyen · 2017-05-18T07:16:01Z

@mtomov rebased turbolinks branch on master, now could you please rebase your branch on top of turbolinks?

mtomov · 2017-05-22T21:54:54Z

Super, thanks, updated from my side as well!

Let me know! Thanks!

tvdeyen · 2017-06-08T20:40:09Z

@mtomov just rebased again with master, could you please rebase again and while your at it fix the typo in the commit message ("Turobolinks" vs. "Turbolinks")? Thanks!

mtomov · 2017-06-08T21:38:48Z

That's that done! All easy with small PRs : ) Thanks!

tvdeyen · 2017-06-09T04:56:54Z

@mtomov Thanks, but I still see the typo in the commit message.

- Enabled for new applications - Disabled for existing applications. Enable in an initializer with: # config/initializers/spree.rb Spree::Backend::Config.configure do |config| config.use_turbolinks = true end - Handles gracefully existing javascript code, which still uses jQuery.ready and gives a warning of the location of those functions.

mtomov · 2017-06-09T07:54:45Z

Correct, my bad, thanks for noticing! Should be good now.

tvdeyen

👌

houndci-bot reviewed May 4, 2017

View reviewed changes

tvdeyen self-assigned this May 9, 2017

tvdeyen requested changes May 9, 2017

View reviewed changes

tvdeyen removed their assignment May 9, 2017

mtomov mentioned this pull request May 9, 2017

Prepare for turbolinks #1900

Merged

tvdeyen force-pushed the turbolinks branch from 5dd8f75 to 78012e5 Compare May 22, 2017 09:03

tvdeyen force-pushed the turbolinks branch from 78012e5 to ba9fefb Compare June 8, 2017 20:38

tvdeyen approved these changes Jun 9, 2017

View reviewed changes

tvdeyen merged commit d524abb into solidusio:turbolinks Jun 9, 2017

mtomov deleted the configure-turbolinks branch June 9, 2017 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure admin turbolinks #1882

Configure admin turbolinks #1882

mtomov commented May 4, 2017 •

edited

Loading

houndci-bot May 4, 2017

tvdeyen May 9, 2017

tvdeyen left a comment •

edited

Loading

tvdeyen May 9, 2017

tvdeyen May 9, 2017 •

edited

Loading

tvdeyen May 9, 2017

tvdeyen May 9, 2017

mtomov May 9, 2017

mtomov May 12, 2017

tvdeyen May 18, 2017

mtomov May 18, 2017

tvdeyen May 22, 2017

tvdeyen May 9, 2017 •

edited

Loading

tvdeyen commented May 18, 2017 •

edited

Loading

mtomov commented May 22, 2017

tvdeyen commented Jun 8, 2017

mtomov commented Jun 8, 2017

tvdeyen commented Jun 9, 2017

mtomov commented Jun 9, 2017

tvdeyen left a comment

		# @!attribute [rw] use_turbolinks
		preference :use_turbolinks, :boolean, default: false

		@@ -0,0 +1,20 @@
		// comment line necessary for correct interpolation of use_turbolinks
		Turbolinks.supported = <%= Spree::Backend::Config.use_turbolinks %>;

Configure admin turbolinks #1882

Configure admin turbolinks #1882

Conversation

mtomov commented May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen May 9, 2017 • edited Loading

Choose a reason for hiding this comment

tvdeyen commented May 18, 2017 • edited Loading

mtomov commented May 22, 2017

tvdeyen commented Jun 8, 2017

mtomov commented Jun 8, 2017

tvdeyen commented Jun 9, 2017

mtomov commented Jun 9, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

mtomov commented May 4, 2017 •

edited

Loading

tvdeyen left a comment •

edited

Loading

tvdeyen May 9, 2017 •

edited

Loading

tvdeyen May 9, 2017 •

edited

Loading

tvdeyen commented May 18, 2017 •

edited

Loading