-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add language selector to nav #158
Conversation
There's a bug in which this method transforms all paths found in frontmatter to relative links, not just image links, breaking any relative links supplied to the nav links list
The default export of LocaleLink will always be set with the active locale, so we don't have to pass down the prop everywhere Closes #138
if present for language selector
This doesn't handle redirecting the news article or resource article pages to the correct translation from the language selector. Right now, it tries to just reload the same page to the equivalent locale's page, but this doesn't work for the article pages: For example: However Not sure if this should be figured out as part of this PR, or if we should redirect to the news index page in that case. Or should changing the language from the selector always just take you to the home page in that language for now? |
@Shadowfiend or @mhluongo any preference to the question posed above? |
We've taken a "forked content" approach to localization- eg some content isn't ever going to exist between locales- so I think your approach makes sense. In an ideal world perhaps the 404 would point people to the localized index? A straight redirect seems like an abuse of HTTP :) |
I'm down with that, we still need to make a 404 page though. :) I see that there already is an issue and design for it! #96 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome, I'm excited! A couple little snaggly bits that could use clarification or an edit - nothing really high conviction though
@@ -4,18 +4,33 @@ import SEO from './SEO.js' | |||
|
|||
import '../css/app.scss' | |||
|
|||
export default (props) => { | |||
const { children, title, description, locale } = props | |||
const LocaleContext = React.createContext({}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been waiting for a good opp to learn about contexts
src/components/LocaleLink.js
Outdated
@@ -20,7 +22,12 @@ const LocaleLink = ({ children, to, locale, ...other }) => | |||
locale = defaultLocale | |||
} | |||
|
|||
localeTo = localeTo.replace('/' + defaultLocale + '/', '/') | |||
// strip any existing locale from the pathname | |||
localeTo = localeTo.replace(/^\/\w{2}\//, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will catch non-locale URLs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... I guess the alternative is either doing a GraphQL query + props in a context somewhere, or testing against all ISO locales...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it bad that it catches non-locale urls? I think they will be ok since LocaleLink
is only used for internal links. The regex should only catch any two letter phrase between two backlashes at the beginning of the path. So the non-locale urls will just pass through. I don't think we'd create any two letter subdirectory? However, I could be very well missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it probably won't... I'm excited for the day we come up with a two-letter content type (id
?) and break this, but I guess it's fine for now. Comments might be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to update this to only test for our supported locales. Lol I think I was just maybe a teeny bit traumatized by our last attempt at that. 🙈 I'll also add more comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
localeTo = localeTo.replace(/^\/\w{2}\//, "/") | ||
|
||
if (locale !== defaultLocale) { | ||
localeTo = `/${locale}${localeTo}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!! Will add comments!
|
||
const Header = props => { | ||
const NavLink = ({ url, label, ...other }) => { | ||
if (/^http/.test(url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used to seeing regex literal tests, took me a second
|
||
// strip prefix build name from url if present | ||
const pathname = | ||
location.pathname.replace(`/${process.env.GATSBY_BRANCH}/`, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little surprised we don't have to do this elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far we've mostly had to add the prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Now that this has been added, I believe there are a bunch of translations specific PRs waiting to be merged which should give you the initial i18n in several languages. |
Yep, this was the last piece @jeremyid. For German, I've been able to roughly spot-check translations myself. Thoughts on how best to do that with these other PRs when we don't have team members who are familiar? Pretty sure @Shadowfiend can cover... we'll, tons. But it'd be nice to have a second review that's not the team |
Includes miscellaneous updates to our header nav:
DropdownComponent
LanguageDropdown
LocaleLink
aware of the active localeTODO: