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

I18n: Switch locale helpers #1466

Open
wants to merge 2 commits into
base: master
from
Open

Conversation

@joallard
Copy link
Contributor

@joallard joallard commented Feb 21, 2015

This is as a follow-up to #1390 and #924.

  • Have the possibility to easily link to localized content in a language-agnostic way, ie. in a layout nav
  • Install a locale switcher link, which would link to the same resource, if available, in the target locale
  • Not relying on the locale dictionary to make this work

In practice, the following helpers are being added:

  • locale_path(page_id, locale = I18n.locale), so we can easily construct eg. a <nav> with language-agnostic keys. (integrated into url_for(*args, locale:) and thus link_to)
  • other_locales and other_locale, some niceties for the following method...
  • switch_locale_path(locale = other_locale), so we can easily switch language while staying on the same resource

Internally,

I recycled @maps that seemed to be useless and used it as a localization map, it would now look like this:

{"president"=>{:en=>"images/president.svg", :es=>"es/images/president.svg"},
 "hello"=>{:en=>"hello.html", :es=>"es/hola.html"},
 "flag"=>{:en=>"images/flag.svg", :es=>"es/images/flag.svg"},
 "index"=>{:en=>"index.html", :es=>"es/index.html"},
 "morning"=>{:es=>"es/manana.html", :en=>"morning.html"},
 "one"=>{:en=>"one.html", :es=>"es/una.html"},
 "partials.index"=>{:en=>"partials/index.html", :es=>"es/partials/index.html"}}

Which makes it easy to find a localized path.

The extension is in bad need of a refactor. I can do some things if the feedback is positive.

I've had to prioritize loading the Frontmatter extension before I18n, because we're using the page_id frontmatter key. Since Routing depends on I18n, this means Frontmatter is now before Routing as opposed to after. Don't know what this means for the rest of the features.

Failing Scenarios:
cucumber features/frontmatter_page_settings.feature:35 # Scenario: Overriding layout through frontmatter

Work in progress; please comment.

@joallard joallard force-pushed the joallard:i18n_paths branch from a38dd7b to c59c7fd Feb 22, 2015
@tdreyno
Copy link
Member

@tdreyno tdreyno commented Feb 23, 2015

Thanks for diving in @joallard. I'm traveling right now, but will try to review and comment soon.

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Feb 24, 2015

You're absolutely right, this extension is really crufty. I've merged in a lot of features, not knowing which are good or bad because I only use i18n in the simplest way, myself.

We'd love any direction, code and ideas you have.

I really like this iterative approach, slowly adding features rather than a complete do-over.

I'm going to push out v4 beta 1 today, so let's save this for beta 2. And we need to track down that failing test.

@joallard
Copy link
Contributor Author

@joallard joallard commented Feb 24, 2015

Awesome. I'll go forward then!

@joallard
Copy link
Contributor Author

@joallard joallard commented Feb 24, 2015

I can't find where the layout option has been implemented. The tests have been added after the implementation. Pointers?

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Feb 24, 2015

Should be this: https://github.com/middleman/middleman/blob/master/middleman-core/lib/middleman-core/core_extensions/front_matter.rb#L44-L45

I think it's unrelated to i18n, but it is related to changing the order things run in.

@joallard
Copy link
Contributor Author

@joallard joallard commented Feb 28, 2015

About that failing test: it's because Routing overrides the Frontmatter layout setting with what's in config.rb. I changed the order to {Routing: 60, FrontMatter: 70, I18n: 80}, but features/capture_html.feature started to fail. I don't know what to do.

@joallard
Copy link
Contributor Author

@joallard joallard commented Apr 3, 2015

Can I get help on this?

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Apr 3, 2015

Haven't had time. capture_html and all the Padrino internals are super gnarly and very prone to breakage. Lets change it so capture_html fails and we'll fix it post-merge.

@tdreyno
Copy link
Member

@tdreyno tdreyno commented May 23, 2015

@joallard Can this be brought up to date with beta2?

@joallard
Copy link
Contributor Author

@joallard joallard commented May 25, 2015

Okay, let me get to it

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Jun 1, 2015

@joallard While you're at it, can you target the v3-stable branch?

@joallard
Copy link
Contributor Author

@joallard joallard commented Jun 2, 2015

Will do

@joallard joallard force-pushed the joallard:i18n_paths branch from c59c7fd to a56d6c9 Jun 2, 2015
@joallard
Copy link
Contributor Author

@joallard joallard commented Jun 2, 2015

Can't do v3-stable. The diff is too large (it's a v4 extension and I was based on master—plus some differences in how the resources are fetched I think) and I don't understand what's there. I lost the context I had in Feb.

I rebased on master, but there are still some failures.

@lightheaded
Copy link

@lightheaded lightheaded commented Aug 10, 2015

Any updates for the issue?

@joallard
Copy link
Contributor Author

@joallard joallard commented Aug 10, 2015

@tdreyno Is it okay if this is a master-only patch? (ie v4 I presume?)

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Aug 10, 2015

Yes

@johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Oct 24, 2015

Does this PR add support for I18n'ed frontmatter keys (as per the title of the PR)? I don't see it in the code...

@joallard
Copy link
Contributor Author

@joallard joallard commented Jan 5, 2016

@tdreyno I'm a little busy at the moment, and my confusion with the failing tests and ext load order (among larger components) has prevented me from being more proactive in fixing this PR, but let me know if there's anything I can do or answer. I'd be happy to help Middleman get a killer i18n any way I can, it's a truly great project you've made you guys.

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Jan 5, 2016

Cool. I'll try rebasing and see where things stand now. Thanks!

@tdreyno tdreyno self-assigned this Jan 5, 2016
@pixelchef
Copy link

@pixelchef pixelchef commented Feb 9, 2016

Any updates on this?

@joallard
Copy link
Contributor Author

@joallard joallard commented Feb 13, 2016

@joallard
Copy link
Contributor Author

@joallard joallard commented Feb 13, 2016

Seeing url_for has been implemented in parallel, here are the parts that this PR is aiming to:

  1. I can obtain the other locales, or the sole other locale easily
  2. Every page has a page_id that is unique, rememberable and independent of locale
  3. I can link to another page in an arbitrary locale (⇒ 2)
  4. I can link to the same page in another locale (⇒ 1, 3)
  5. I can change the page_id in a page frontmatter
@joallard joallard force-pushed the joallard:i18n_paths branch from a56d6c9 to e134fb2 Feb 13, 2016
@joallard
Copy link
Contributor Author

@joallard joallard commented Feb 13, 2016

What I'm tiptoeing around is the page_id logic. That mechanism isn't really solid, and it seems to me the urls in general should be able to refer to a certain page_id, a little à la how Rails has as labels for routing.

(Btw, the rebase fixed the extension priorities that seem to have magically been sorted out)

@joallard joallard changed the title I18n: Localized paths helper and frontmatter keys I18n: Switch locale and frontmatter page id Feb 13, 2016
@joallard joallard force-pushed the joallard:i18n_paths branch from e134fb2 to 00494f6 Oct 19, 2016
@joallard joallard force-pushed the joallard:i18n_paths branch 2 times, most recently from a80c139 to 853e90f Oct 19, 2016
@joallard joallard force-pushed the joallard:i18n_paths branch from 853e90f to c70c7ec Oct 20, 2016
@joallard
Copy link
Contributor Author

@joallard joallard commented Oct 20, 2016

Whew! And here we are. Works. Rebased. That required concentration.

As we can see (I activated verbose commit mode), it pretty much comes in one piece. It's dependent on #1996, so we'll solve that first.

What's left:

  • That switch_locale_path helper
  • Clean up url_for
  • Clean up Git history

We'll probably get some edge cases too to add in there. Maybe @tdreyno you can give me some tips or things to watch out for for that url_for method, it's pretty... ugly and contorted. And what's with the "relative" stuff?

@joallard
Copy link
Contributor Author

@joallard joallard commented Oct 20, 2016

Also, is there a reason we don't use Pry out of the box in dev?

@joallard
Copy link
Contributor Author

@joallard joallard commented Oct 20, 2016

Next up: remove the (*) localizable folder. With I18n activated, we'll assume a page is localizable if it's html, unless it's got localizable: false in it.

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Oct 26, 2016

@joallard I think because we used to support versions of Ruby which didn't support pry... but now, good to include by default

@joallard joallard force-pushed the joallard:i18n_paths branch 2 times, most recently from 7dd882b to 89c9437 Nov 13, 2016
@joallard joallard force-pushed the joallard:i18n_paths branch from 89c9437 to c47e189 Nov 18, 2016
@manuelmeurer
Copy link
Contributor

@manuelmeurer manuelmeurer commented Feb 17, 2017

Hi @joallard and @tdreyno, what it the status of this PR? I just ran into the issue that link_to/url_for with I18n doesn't work as expected (at least with directory_indexes, I didn't check without). Will the changes in this PR fix this? What's needed to move forward?

@johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Feb 18, 2017

FYI here's my I18n helper methods I've added to my Middleman setup: https://gist.github.com/johnnyshields/98a695df51b1e99f3593579d3c9a3fd1

One thing to note is that my primary locale is Japanese, so my site root (/) is Japanese, and other locales are at /en/... etc. Would be good if this can be support in this PR.

@joallard
Copy link
Contributor Author

@joallard joallard commented Feb 18, 2017

I had a refactor lingering, it's pushed at e67608d. @tdreyno you'll probably notice that I've got references to the i18n in my classes everywhere, should this have been structs?

(The refactor commit has failures, but the first commit is good iirc. This is not exactly priority no.1 for me at the moment, but I wanted to share my work so someone could build off of it in case.)

@joallard joallard force-pushed the joallard:i18n_paths branch from 6e89a36 to e67608d Feb 18, 2017
@jnf-si
Copy link

@jnf-si jnf-si commented Jun 1, 2017

Hi! I just wanted to take a moment to thank yinz for the ongoing work on this issue. This (year-long!) thread has been really informative and has helped me figure out how to move forward with my Middleman project. I also really appreciate the supportive tone and graceful acknowledgement that OSS is hard, language is hard, and people are busy.

Thanks again for sticking with this. 👍

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Jun 1, 2017

Looking for a passing build, but happy to review whenever

@joallard
Copy link
Contributor Author

@joallard joallard commented Jun 1, 2017

I don't have so much time to dedicate to this right now, but it should be only a matter of fixing a few loose ends on the tip of the branch. I'm happy to help if someone wants to pick it back up.

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Jun 1, 2017

That liquid error is a whole other thing I need to clean up (likely by removing tests for old versions of liquid), you can ignore it

@kstratis
Copy link

@kstratis kstratis commented Oct 31, 2017

Hi, any update on this one?

@dorellang
Copy link

@dorellang dorellang commented Jan 8, 2018

I've been hacking Middleman a lot (by monkey patching and reopening some framework classes) to get Internationalization, Blogging and Directory Indexes playing along for my personal website. I think I grasp how the code currently on master works and I'd love to contribute, but I don't know what the project standards are, how the code should be tested, if there's any backwards compatible behavior that should not be broken, etc.

I'd really appreciate any information. I'm looking forward to solving this issue, since the solution I've made for my website just feels like a complete duct tape mess and I'm pretty sure that would break in the next Middleman release. It just would be freaking nice to get it working in a cleaner way.

Cheers!

@ChristianPeters
Copy link

@ChristianPeters ChristianPeters commented Jan 14, 2018

I also spent a lot of time, having to understand nearly all messy internals to be able to duct tape our locale switch helpers together.
Sadly, I can't find any time to contribute but I could share my helper structure if that might be helpful.

@dagumak
Copy link

@dagumak dagumak commented Sep 13, 2018

Any updates?

@tdreyno
Copy link
Member

@tdreyno tdreyno commented Sep 20, 2018

@dorellang If you can write a specification of how it should work, we can convert that to a Cucumber test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.