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

Factor out a <LocaleLink> component. #131

Merged
merged 3 commits into from
Aug 13, 2020
Merged

Factor out a <LocaleLink> component. #131

merged 3 commits into from
Aug 13, 2020

Conversation

toolness
Copy link
Contributor

This factors out a <LocaleLink> component that automatically prefixes the passed-in URL with the current locale, as in WoW (see JustFixNYC/who-owns-what#186).

* the path!
*/
export function LocaleLink<TState>(
props: GatsbyLinkProps<TState> & { ref?: never }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was having weird type issues with the ref prop but since we never actually use it in our code, I just simplified things by prohibiting it.

Copy link
Member

@sraby sraby left a comment

Choose a reason for hiding this comment

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

Wow this is such a great feeling to see all of these locale props disappear! This looks great to me, thank you Atul!

Comment on lines +47 to +53
content: {
title: string;
subtitle: string;
ctaLink: string;
ctaText: string;
};
};
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach of slowly adding in more explicit type definitions as we go.

@sraby
Copy link
Member

sraby commented Aug 13, 2020

@toolness feel free to squash and merge when you get a chance!

@toolness toolness merged commit e473495 into master Aug 13, 2020
@toolness toolness deleted the locale-link branch August 13, 2020 12:29
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.

2 participants