Implement internationalization based on URL path not query string

nihonjinrxs commented 5 years ago

Resolves #74

Description

This modifies the way locale is handled for internationalization in the following way:

Locale is specified as a scope on the route, and thus a prefix on the URL
Default scope is implemented via a redirect prior to the locale scope in the routes file (using a path glob, which should work for any existing route, but it's failing the request tests... see below)
Locale links have been added that reload the current page with the selected locale (updating the route along the way)

I've tested this a variety of ways, and all seems to work under normal online circumstances.

@bhaibel @LizPrescott @that-jill I'll need help understanding how this can help us to do offline work. If you all can dive into this PR and validate that it will actually help us with the service worker situation, I'd appreciate it.

I'm using Rack::Rewrite to enable non-locale-scoped paths to load as normal, but load with the default locale in context, so that generated links/routes from that page will include the default context. This seemed like the cleanest way to achieve that. Happy to discuss if that causes problems.

Also, for anyone else reviewing, I'm not a UI person. I'm sure there's a better way of doing that partial view in app/views/shared/_locale_links.html.erb, but I'm rusty on my rails view helpers and couldn't figure it out. It's also 2am, so maybe that's part of it. Anyway, help is welcomed there too.

Type of change

Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update -- maybe for offline stuff? don't know.

How Has This Been Tested?

One remaining request test is failing, but that's because what it's testing is not correct. (New dive POST.) That's outside the scope of this change, and I don't think I broke that, so I'm leaving it for now.

There's another test for locale-context homepage renders that I added but left skipped. It's brittle at the moment, and I'd like to work on it more before enabling it. I'm leaving it there so there's a reminder to do something on it.

Screenshots

LizPrescott commented 5 years ago

It looks to me like the request specs are failing because the test urls don't include the locale part of the url yet.

nihonjinrxs commented 5 years ago

@LizPrescott I think the service worker is going to have to allow for prefixed paths, or at least assume a locale in the beginning of the path. Putting the locale at the end seems difficult and brittle at best, given the various depths of route URLs we might generate in the app.

@bhaibel I'd appreciate a review on this from you if you have some time, particularly in reference to the interaction between locale/routing and the service worker.

cflipse commented 5 years ago

Also, the i18n standard I've generally observed is that the language does go to the front of the locale.

I suppose another option might be to make it subdomain based? So, it's fr.coral.com/nursery_tables or something like that?

nihonjinrxs commented 5 years ago

@LizPrescott Thoughts? I think you mentioned somewhere that it doesn't actually break things?

LizPrescott commented 5 years ago

Let me double check tonight.

On Aug 23, 2019, at 12:44 PM, Ryan B. Harvey notifications@github.com wrote:

@LizPrescott Thoughts? I think you mentioned somewhere that it doesn't actually break things?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

LizPrescott commented 5 years ago

Ok I just double checked. Unfortunately, this does break the service worker, and I'm not sure why tbh.

LizPrescott commented 5 years ago

Also, the i18n standard I've generally observed is that the language does go to the front of the locale.

I suppose another option might be to make it subdomain based? So, it's fr.coral.com/nursery_tables or something like that?

This seems like a good one to try!

LizPrescott commented 4 years ago

Shall we try this gem? @cflipse @nihonjinrxs https://github.com/semaperepelitsa/subdomain_locale

cflipse commented 4 years ago

@LizPrescott sure, that gem looks interesting, could work

rubyforgood / coral