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

dialect overflow e.g. zh-TW #2803

Closed
wants to merge 6 commits into from

Conversation

arthurtalkgoal
Copy link
Contributor

No description provided.


def truncate_locale(locale)
locale =~ /\-/ ? locale.upcase.to_s.split('-')[1] : locale.upcase

Copy link
Member

Choose a reason for hiding this comment

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

✂️

@simi
Copy link
Member

simi commented Dec 10, 2014

Hello @arthurtalkgoal. Can you please provide describing spec for this helper?

@simi
Copy link
Member

simi commented Dec 10, 2014

Also can you merge master in or rebase?

Conflicts:
	pages/app/views/refinery/admin/pages/_page.html.erb
@arthurtalkgoal
Copy link
Contributor Author

Sure! just found that mine isn't merged

@simi
Copy link
Member

simi commented Dec 10, 2014

If I understand well, this is separating country from locale code. If that's correct, I thinkt it will be good to rename truncate_locale method to locale_country or similar. Does it make sense?

<li<%= %Q{ class=selected} if locale.to_s == local_assigns[:current_locale].to_s %>>
<%= link_to refinery.url_for(:switch_locale => locale, :parent_id => params[:parent_id]), id: locale do %>
<div class="locale_icon">
<%= refinery_icon_tag('locale.svg', :size => '24x24') %>
<span class="code"><%= locale.upcase %></span>
<%= refinery_icon_tag('locale.svg', :size => '24x24', :alt => locale.upcase) %>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not use locale_country here also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as below reply. I applied full text for alt. And I am ok to apply locale_country anywhere if applicable.

@simi
Copy link
Member

simi commented Dec 10, 2014

I've added few comments. Sorry for being so pedantic.

@simi
Copy link
Member

simi commented Dec 10, 2014

Please can you strip those whitespace changes @arthurtalkgoal? I'll merge after.

@@ -5,6 +5,11 @@ def current_admin_locale
# TODO: move current_admin_locale to Refinery::I18n
::I18n.locale
end

def locale_country(locale)
locale =~ /\-/ ? locale.upcase.to_s.split('-')[1] : locale.upcase
Copy link
Member

Choose a reason for hiding this comment

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

We can always use split, in conjunction with [-1]

def locale_country(locale)
  locale.upcase.to_s.split('-')[-1]
end
irb(main):001:0> 'en-US'.split('-')[-1]
=> "US"
irb(main):002:0> 'en'.split('-')[-1]
=> "en"

Copy link
Member

Choose a reason for hiding this comment

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

actually it should be

locale.to_s.upcase.split('-').last

Copy link
Member

Choose a reason for hiding this comment

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

@simi wins

flawless victory

@arthurtalkgoal
Copy link
Contributor Author

locale.to_s.upcase.split('-').last is a smart ways to save codes!


def locale_country(locale)
locale.to_s.upcase.split('-').last

Copy link
Member

Choose a reason for hiding this comment

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

✂️ ?

@parndt
Copy link
Member

parndt commented Dec 11, 2014

Seems like a good option to configure your editor to strip out trailing whitespace. In sublime text, you can do this with this preference:

"trim_trailing_white_space_on_save": true

In fact here's my entire preferences file:

{
    "bold_folder_labels": true,
    "color_scheme": "Packages/Theme - Flatland/Flatland.tmtheme",
    "draw_white_space": "selection",
    "ensure_newline_at_eof_on_save": true,
    "file_exclude_patterns":
    [
        ".DS_Store",
        ".travis_status~"
    ],
    "folder_exclude_patterns":
    [
        ".git",
        ".rbx",
        "tmp",
        "log",
        "doc",
        ".bundle",
        "coverage",
        ".vagrant"
    ],
    "font_face": "Monaco",
    "font_options":
    [
        "subpixel_antialias"
    ],
    "font_size": 15.0,
    "highlight_line": true,
    "ignored_packages":
    [
        "Vintage"
    ],
    "rulers":
    [
        80,
        120
    ],
    "save_on_focus_lost": true,
    "scroll_past_end": true,
    "tab_completion": true,
    "tab_size": 2,
    "theme": "Flatland.sublime-theme",
    "translate_tabs_to_spaces": true,
    "trim_trailing_white_space_on_save": true
}

@parndt
Copy link
Member

parndt commented Dec 11, 2014

Thanks for your work on this; would really appreciate if it were squashed down to one commit. Let me know if this is a problem for you and I can do it instead.

Is this going to be a problem that zh-TW will become just TW? (As that is a country, and not a locale)

@simi
Copy link
Member

simi commented Dec 11, 2014

🎺 squash'n'merge 🌐

@arthurtalkgoal
Copy link
Contributor Author

Hi parndt,

I use vim or most of the time Windows Notepad++, I am not sure how the trailing_white_space created.

I will check it next time. Would you mind combine it into a commit for me?

re zh-TW to TW:
The locale code is in fact zh-TW, it won't work if converted to "TW" in codes/locale setting.

But for display purpose, we understand "TW" stands for Taiwan and Traditional Chinese; "CN" for simiplied chinese. and HK for Hong kong chinese etc.

@simi
Copy link
Member

simi commented Dec 11, 2014

@arthurtalkgoal should I squash it for you?

@arthurtalkgoal
Copy link
Contributor Author

@simi, yes, please.

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.

None yet

3 participants