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

Localize each store #95

Merged

Conversation

DanielePalombo
Copy link
Contributor

@DanielePalombo DanielePalombo commented Jun 23, 2017

Modified the admin general settings page to select different available locales to each stores

rel #82

@DanielePalombo DanielePalombo changed the title Localize every store Localize each store Jun 23, 2017
@DanielePalombo DanielePalombo force-pushed the nebulab/localize-every-store branch from 9291b8d to 674f538 Compare June 23, 2017 20:26
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 like the change. I have some comments about the implementation. I think we can make use of handlebars templates here.

}
});

$(function(){
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use jQuery DOM ready events. Use Spree.ready, so it is Turbolinks compatible.

And maybe we should put this in the view where we actually have the DOM elements available. Initializing directly inside the backbone file makes changes to JS initializing very hard.

<table class="index locales-settings">
<thead>
<tr>
<th>Name</th>
Copy link
Member

Choose a reason for hiding this comment

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

Please use

Spree::Store.human_attribute_name_for(:name)

<thead>
<tr>
<th>Name</th>
<th>Url</th>
Copy link
Member

Choose a reason for hiding this comment

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

ditto

<%= link_to '', '#', :class => 'fa fa-edit edit-available-locales icon_link icon-edit no-text with-tip', :data => {store_id: store.id, action: 'edit'}, :title => Spree.t('edit') %>
</td>
</tr>
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

I think a handlebars template would be much better here.

Very sad we can't make use of editable-table introduced in solidusio/solidus/pull/1580 here, because of back wards compatibility, but maybe the same implementation can be used here?

@DanielePalombo DanielePalombo force-pushed the nebulab/localize-every-store branch from 674f538 to 351ad27 Compare July 14, 2017 13:52
@DanielePalombo
Copy link
Contributor Author

DanielePalombo commented Jul 14, 2017

PR updated.

Thanks for your review, with your suggestions the code is more simple and readable.
Sorry for taking some time but it has been the first time that I have used backbone and handlebars, I hope that everything is ok!

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.

just some change requested, general direction is very good!

{{/each}}
{{/if}}
</td>
<td class="actions">
Copy link
Member

Choose a reason for hiding this comment

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

can we use {{#if editing}} to conditionally display actions instead of hiding/showing with css?

<% end %>
<script>
$(function(){
if ($(".locales-settings").length) {
Copy link
Member

Choose a reason for hiding this comment

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

can we move this in some JS file?

@@ -18,6 +18,12 @@ def all_locales_options
SolidusI18n::Locale.all.map { |locale| locale_presentation(locale) }.push(['English (EN)', :en])
end

def available_locales_presentation(store)
Copy link
Member

Choose a reason for hiding this comment

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

is this method used?

@tvdeyen
Copy link
Member

tvdeyen commented Sep 13, 2017

@DanielePalombo there seems to be an related issue on latest master of Solidus. Mind to take a look into it?

'click [data-action=cancel]': 'onCancel'

onEdit: (e) ->
return if @$el.hasClass('editing')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use the editing value for the conditional, that way we can stop adding and removing class to the element

@tvdeyen tvdeyen added this to the 2.0 milestone Feb 7, 2018
@kennyadsl kennyadsl force-pushed the nebulab/localize-every-store branch 2 times, most recently from a4b9ceb to 6dfc5dc Compare February 9, 2018 10:34
DanielePalombo and others added 15 commits March 1, 2018 15:53
Change the specs to work with multi store available locales
Add handlebars templates and helpers to manage the inline editing of
available languages
This Object toggles the editing mode for the rows and save the new value
through the API.
This Object append the `tr` inside the tbody and run the
EditInlineLocales on the new row.
auto width resolution was breaking table, this will make the select2
not change its size when opened
@kennyadsl kennyadsl force-pushed the nebulab/localize-every-store branch from d386b1c to 3803c36 Compare March 1, 2018 14:54
@kennyadsl kennyadsl merged commit c56d5ff into solidusio:master Mar 1, 2018
@kennyadsl kennyadsl deleted the nebulab/localize-every-store branch March 1, 2018 15:39
@alepore
Copy link
Contributor

alepore commented Mar 21, 2018

Should we deprecate the old global setting or update the README?

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.

5 participants