Skip to content

Implement internationalization based on URL path not query string#77

Open
nihonjinrxs wants to merge 4 commits intomasterfrom
i18n-path-based
Open

Implement internationalization based on URL path not query string#77
nihonjinrxs wants to merge 4 commits intomasterfrom
i18n-path-based

Conversation

@nihonjinrxs
Copy link
Copy Markdown
Collaborator

@nihonjinrxs nihonjinrxs commented Aug 17, 2019

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
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator

@LizPrescott LizPrescott left a comment

Choose a reason for hiding this comment

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

Tested it and found this also seems to break the offline stuff.

Can you put the /en or /fr at the end of the url? I think that might get it back into the scope of the service worker.

@nihonjinrxs
Copy link
Copy Markdown
Collaborator Author

@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.

@nihonjinrxs nihonjinrxs marked this pull request as ready for review August 20, 2019 03:18
@cflipse
Copy link
Copy Markdown
Collaborator

cflipse commented Aug 23, 2019

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
Copy link
Copy Markdown
Collaborator Author

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

@LizPrescott
Copy link
Copy Markdown
Collaborator

LizPrescott commented Aug 23, 2019 via email

@LizPrescott
Copy link
Copy Markdown
Collaborator

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

@LizPrescott
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Collaborator

LizPrescott commented Oct 19, 2019

@cflipse
Copy link
Copy Markdown
Collaborator

cflipse commented Oct 24, 2019

@LizPrescott sure, that gem looks interesting, could work

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.

Fix i8n in a way that does not conflict with service workers

3 participants