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

Configure admin turbolinks #1882

Merged
merged 1 commit into from
Jun 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/app/assets/javascripts/spree/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
//= require spree/backend/namespaces
//= require spree/backend/translation
//= require spree/backend/backbone-overrides
//= require spree/backend/turbolinks-configuration
//= require spree/backend/format_money
//
//= require spree/backend/templates
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// comment line necessary for correct interpolation of use_turbolinks
Turbolinks.supported = <%= Spree::Backend::Config.use_turbolinks %>;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@mtomov updated solidusio/turbolinks


Spree.jQueryReady = $.fn.ready;

// override jQuery.ready to use Spree.ready even if it was not used explicitly
$.fn.ready = function (callback) {
console.warn("jQuery.ready() is deprecated. Use Spree.ready() instead. Called from:", callback );
Spree.ready(callback);
};

Spree.ready = function(callback) {
if (Turbolinks.supported) {
jQuery(document).on('turbolinks:load', function() {
callback(jQuery);
});
} else {
Spree.jQueryReady(callback);
}
};
3 changes: 3 additions & 0 deletions backend/app/models/spree/backend_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ module Spree
class BackendConfiguration < Preferences::Configuration
preference :locale, :string, default: Rails.application.config.i18n.default_locale

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

ORDER_TABS ||= [:orders, :payments, :creditcard_payments,
:shipments, :credit_cards, :return_authorizations,
:customer_returns, :adjustments, :customer_details]
Expand Down
9 changes: 9 additions & 0 deletions backend/spec/models/spree/backend_configuration_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
require 'spec_helper'

describe Spree::BackendConfiguration, type: :model do
let(:prefs) { Spree::Backend::Config }

describe '#use_turbolinks' do
specify { expect(prefs).to respond_to(:use_turbolinks) }
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
config.use_static_preferences!

config.locale = 'en'
config.use_turbolinks = true
end
<% end -%>

Expand Down