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.

Sign up for GitHub

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

Jump to bottom

feat(i18n): Setup react-i18next #9797

Merged
merged 5 commits into from
Jul 18, 2022
Merged

feat(i18n): Setup react-i18next #9797

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24e2f21
feat: add i18next
araujobarret Mar 30, 2022
e4f424e
Ensure json files are excluded in jest config
lidimayra Feb 15, 2022
b5a4584
Try to remove the beginning of the regex on config
lidimayra Jul 18, 2022
1b51258
Test actual strings intead of keys
lidimayra Jul 18, 2022
501ee36
simplifying (#10547)
pvinis Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jest.config.v2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module.exports = {
testURL: "http://localhost",
transform: {
"\\.(gql|graphql)$": "jest-transform-graphql",
".(ts|tsx|js|jsx)": "babel-jest",
"^.+\\.(ts|tsx|js|jsx)$": "babel-jest",
Copy link
Contributor

Choose a reason for hiding this comment

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

this regex feels weird.. the $ at the end makes sense, and should be enough to exclude the json files. the beginning part seems unnecessary?

Copy link
Contributor Author

@lidimayra lidimayra Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Applied here: b5a4584

},
},
],
Expand Down
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@
"helmet": "2.3.0",
"http-proxy": "1.18.1",
"http-shutdown": "1.2.1",
"i18next": "^21.6.14",
"i18next-browser-languagedetector": "^6.1.4",
"insane": "2.6.1",
"ip": "^1.1.5",
"isomorphic-fetch": "^2.2.1",
"jsdom": "11.6.2",
Expand Down Expand Up @@ -166,6 +169,7 @@
"react-dropzone": "^11.4.2",
"react-head": "3.0.2",
"react-lines-ellipsis": "^0.14.0",
"react-i18next": "^11.16.2",
"react-linkify": "1.0.0-alpha",
"react-relay": "10.1.3",
"react-relay-network-modern": "4.7.3",
Expand Down
86 changes: 60 additions & 26 deletions src/v2/Components/NavBar/NavBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { useNavBarHeight } from "./useNavBarHeight"
import { RouterLink } from "v2/System/Router/RouterLink"
import { useTracking } from "react-tracking"
import { Z } from "v2/Apps/Components/constants"
import { useTranslation } from "react-i18next"

/**
* Old Force pages have the navbar height hardcoded in several places. If
Expand Down Expand Up @@ -72,6 +73,7 @@ export const NavBar: React.FC = track(
return null
}
const { trackEvent } = useTracking()
const { t } = useTranslation()
const [showMobileMenu, toggleMobileNav] = useState(false)
const xs = __internal__useMatchMedia(themeProps.mediaQueries.xs)
const sm = __internal__useMatchMedia(themeProps.mediaQueries.sm)
Expand Down Expand Up @@ -108,7 +110,7 @@ export const NavBar: React.FC = track(
event: React.MouseEvent<HTMLAnchorElement, MouseEvent>
) => {
const link = event.currentTarget
const text = (link.textContent || link.getAttribute("data-label")) ?? ""
const text = (link.getAttribute("data-label") || link.textContent) ?? ""
const href = link.getAttribute("href")!

trackEvent({
Expand Down Expand Up @@ -149,7 +151,7 @@ export const NavBar: React.FC = track(
flex={1}
size="small"
>
Sign Up
{t`navbar.signup`}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

</Button>

<Button
Expand All @@ -160,7 +162,7 @@ export const NavBar: React.FC = track(
ml={1}
size="small"
>
Log In
{t`navbar.login`}
</Button>
</Flex>
)}
Expand All @@ -181,34 +183,38 @@ export const NavBar: React.FC = track(
href="/collect"
textDecoration="none"
onClick={handleClick}
data-label="Buy"
>
Buy
{t("navbar.buy")}
</NavBarItemLink>
</Flex>

<NavBarItemLink
href="/sell"
textDecoration="none"
onClick={handleClick}
data-label="Consign"
>
Sell
{t("navbar.sell")}
</NavBarItemLink>

<NavBarItemLink
href="/price-database"
textDecoration="none"
onClick={handleClick}
data-label="Price Database"
>
Price Database
{t`navbar.priceDatabase`}
</NavBarItemLink>

<Flex alignItems="center" display={["none", "none", "flex"]}>
<NavBarItemLink
href="/articles"
textDecoration="none"
onClick={handleClick}
data-label="Articles"
>
Editorial
{t`navbar.editorial`}
</NavBarItemLink>
</Flex>
</Text>
Expand All @@ -230,7 +236,7 @@ export const NavBar: React.FC = track(
})
}}
>
Log In
{t`navbar.login`}
</Button>

<Button
Expand All @@ -245,7 +251,7 @@ export const NavBar: React.FC = track(
})
}}
>
Sign Up
{t`navbar.signup`}
</Button>
</Flex>
)}
Expand Down Expand Up @@ -325,7 +331,7 @@ export const NavBar: React.FC = track(
onClick={handleClick}
data-label="Artists"
/>
Artists
{t`navbar.artists`}
</NavBarItemButton>
)}
</Dropdown>
Expand Down Expand Up @@ -356,37 +362,65 @@ export const NavBar: React.FC = track(
onClick={handleClick}
data-label="Artworks"
/>
Artworks
{t`navbar.artworks`}
</NavBarItemButton>
)}
</Dropdown>

<NavBarItemLink href="/auctions" onClick={handleClick}>
Auctions
<NavBarItemLink
href="/auctions"
onClick={handleClick}
data-label="Auctions"
>
{t`navbar.auctions`}
</NavBarItemLink>

<NavBarItemLink href="/viewing-rooms" onClick={handleClick}>
Viewing&nbsp;Rooms
<NavBarItemLink
href="/viewing-rooms"
onClick={handleClick}
data-label="Viewing Rooms"
>
{t`navbar.viewingRooms`}
</NavBarItemLink>

<NavBarItemLink href="/galleries" onClick={handleClick}>
Galleries
<NavBarItemLink
href="/galleries"
onClick={handleClick}
data-label="Galleries"
>
{t`navbar.galleries`}
</NavBarItemLink>

<NavBarItemLink href="/art-fairs" onClick={handleClick}>
Fairs
<NavBarItemLink
href="/art-fairs"
onClick={handleClick}
data-label="Fairs"
>
{t`navbar.fairs`}
</NavBarItemLink>

<NavBarItemLink href="/shows" onClick={handleClick}>
Shows
<NavBarItemLink
href="/shows"
onClick={handleClick}
data-label="Shows"
>
{t`navbar.shows`}
</NavBarItemLink>

<NavBarItemLink href="/institutions" onClick={handleClick}>
Museums
<NavBarItemLink
href="/institutions"
onClick={handleClick}
data-label="Institutions"
>
{t`navbar.museums`}
</NavBarItemLink>

<NavBarItemLink href="/nft" onClick={handleClick}>
NFTs
<NavBarItemLink
href="/nft"
onClick={handleClick}
data-label="NFTs"
>
{t`navbar.nfts`}
</NavBarItemLink>
</Flex>

Expand All @@ -402,7 +436,7 @@ export const NavBar: React.FC = track(
})
}}
>
Download App
{t`navbar.downloadApp`}
</NavBarItemButton>
</Flex>
</Text>
Expand Down
8 changes: 4 additions & 4 deletions src/v2/Components/NavBar/__tests__/NavBar.jest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,17 @@ describe("NavBar", () => {
describe("desktop", () => {
it("renders logged out items", () => {
const wrapper = getWrapper()
expect(wrapper.html()).toContain("Log In")
expect(wrapper.html()).toContain("Sign Up")
expect(wrapper.html()).toContain("navbar.login")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is strange.. should it contain the key? feels like the test should be testing if the actual text is there.

something like

it("works in English", () => {
  expect(getWrapper().html()).toContain("Log In")
})
it("works in Greek", () => {
  someFuncToMockBrowserLang("gr")
  expect(getWrapper().html()).toContain("Σύνδεση")
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is something that we also discussed during the implementation... but if the goal here is that at some point the language experts should be capable of using a third-party service to update the translations as they wish, do we want the tests to fail when the content of text changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

i would say yes 🤔.

i guess we should support both? it would be great to be able to test the actual text we expand to, to test that languages are actually applied correctly, but also we should allow for key strings to be checked, though that can just be the testID 🤔. Im not set on one or the other.

Maybe we start with something and see how it goes. I just feel like checking just for the key might take us to a place where a translation is missing, and we never know, because our test is green, and our website says navbar.signup instead of Sign Up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a super valid point
I added 1b51258 to change the test to look for the strings instead of keys

expect(wrapper.html()).toContain("navbar.signup")
expect(wrapper.find(BellIcon).length).toEqual(0)
expect(wrapper.find(SoloIcon).length).toEqual(0)
})

it("renders logged in items", () => {
// @ts-expect-error PLEASE_FIX_ME_STRICT_NULL_CHECK_MIGRATION
const wrapper = getWrapper({ user: true })
expect(wrapper.html()).not.toContain("Log In")
expect(wrapper.html()).not.toContain("Sign Up")
expect(wrapper.html()).not.toContain("navbar.login")
expect(wrapper.html()).not.toContain("navbar.signup")
expect(wrapper.find(BellIcon).length).toEqual(1)
expect(wrapper.find(SoloIcon).length).toEqual(1)
})
Expand Down
37 changes: 23 additions & 14 deletions src/v2/Components/Search/SearchBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import * as Schema from "v2/System/Analytics/Schema"
import { SystemQueryRenderer } from "v2/System/Relay/SystemQueryRenderer"
import {
FirstSuggestionItem,
PLACEHOLDER,
PLACEHOLDER_XS,
SuggestionItem,
} from "v2/Components/Search/Suggestions/SuggestionItem"
import { Router } from "found"
Expand All @@ -28,10 +26,11 @@ import { useDidMount } from "v2/Utils/Hooks/useDidMount"
import createLogger from "v2/Utils/logger"
import { Media } from "v2/Utils/Responsive"
import { SearchInputContainer } from "./SearchInputContainer"
import { withTranslation, WithTranslation } from "react-i18next"

const logger = createLogger("Components/Search/SearchBar")

export interface Props extends SystemContextProps {
export interface Props extends SystemContextProps, WithTranslation {
Copy link
Contributor

Choose a reason for hiding this comment

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

why would we do this here this way, and not with the same useTranslation hook?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i see.. this is a class component.

even though it makes sense, I would much prefer it if we used something like https://github.com/artsy/eigen/blob/26a98c408c223efefbb0b8309448c3bb79256ab3/src/app/Scenes/Search/components/CityGuideCTA.tsx#L12-L13 (docs here at the bottom https://github.com/artsy/eigen/blob/b4484db7fb7ddfbad4e903c993628bbd90a909b3/docs/palette_v3_migration_guide.md)
so we can keep everything nice and centralized, with a t exposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pvinis !! ❤️

As this is not part of an actual project, I don't think I'll have time to focus a lot on these changes before Wednesday. Do you think it would make sense to remove the SearchBar translations from this PR so we could make it smaller by applying the i18n to the NavBar only? And then working on the SearchBar on the next Future Friday?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think the way I suggest is a good way to do this change for the file here, I'm happy to help or even make a PR on top of this one with the change. If you think we need more discussion on this, I'll gladly greenlight this 👍.

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 completely agree that your suggestion would be much cleaner, if it's possible to open a PR on top of this one, that would be great. I just don't want to put any pressure on you either, I'm already super grateful for the careful code review. ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a quick stab at it.

relay: RelayRefetchProp
router?: Router
viewer: SearchBar_viewer
Expand Down Expand Up @@ -373,7 +372,7 @@ export class SearchBar extends Component<Props, State> {

renderAutosuggestComponent({ xs }) {
const { term } = this.state
const { viewer } = this.props
const { viewer, t } = this.props

const inputProps = {
"aria-label": "Search Artsy",
Expand All @@ -390,7 +389,7 @@ export class SearchBar extends Component<Props, State> {
event.preventDefault()
}
},
placeholder: xs ? PLACEHOLDER_XS : PLACEHOLDER,
placeholder: xs ? t`navbar.searchArtsy` : t`navbar.searchBy`,
value: term,
}

Expand Down Expand Up @@ -465,7 +464,7 @@ export class SearchBar extends Component<Props, State> {
}

export const SearchBarRefetchContainer = createRefetchContainer(
withSystemContext(SearchBar),
withSystemContext(withTranslation()(SearchBar)),
{
viewer: graphql`
fragment SearchBar_viewer on Viewer
Expand Down Expand Up @@ -509,35 +508,43 @@ export const SearchBarRefetchContainer = createRefetchContainer(
* Displays during SSR render, but once mounted is swapped out with
* QueryRenderer below.
*/
const StaticSearchContainer: React.FC<{ searchQuery: string } & BoxProps> = ({
searchQuery,
...rest
}) => {
const StaticSearchContainer: React.FC<
{ searchQuery: string } & BoxProps & WithTranslation
> = ({ searchQuery, t, ...rest }) => {
return (
<>
<Box display={["block", "none"]} {...rest}>
<SearchInputContainer
placeholder={searchQuery || PLACEHOLDER_XS}
placeholder={searchQuery || t`navbar.searchArtsy`}
defaultValue={searchQuery}
/>
</Box>

<Box display={["none", "block"]} {...rest}>
<SearchInputContainer
placeholder={searchQuery || PLACEHOLDER}
placeholder={searchQuery || t`navbar.searchBy`}
defaultValue={searchQuery}
/>
</Box>
</>
)
}

const WithTranslationStaticSearchContainer = withTranslation()(
StaticSearchContainer
)

export const SearchBarQueryRenderer: React.FC<BoxProps> = props => {
const { relayEnvironment, searchQuery = "" } = useContext(SystemContext)
const isMounted = useDidMount(typeof window !== "undefined")

if (!isMounted) {
return <StaticSearchContainer searchQuery={searchQuery} {...props} />
return (
<WithTranslationStaticSearchContainer
searchQuery={searchQuery}
{...props}
/>
)
}

return (
Expand All @@ -561,7 +568,9 @@ export const SearchBarQueryRenderer: React.FC<BoxProps> = props => {
// from within the NavBar (it's not a part of any app) we need to lean
// on styled-system for showing / hiding depending upon breakpoint.
} else {
return <StaticSearchContainer searchQuery={searchQuery} />
return (
<WithTranslationStaticSearchContainer searchQuery={searchQuery} />
)
}
}}
/>
Expand Down
3 changes: 0 additions & 3 deletions src/v2/Components/Search/Suggestions/SuggestionItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,6 @@ const SuggestionItemLink = styled(RouterLink).attrs({
min-height: 60px;
`

export const PLACEHOLDER = "Search by artist, gallery, style, theme, tag, etc."
export const PLACEHOLDER_XS = "Search Artsy"
Comment on lines -87 to -88
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These weren't being used anywhere besides inside the SearchBar component, so we deleted it from here.


const DefaultSuggestion: React.FC<SuggestionItemProps> = ({
display,
label,
Expand Down
1 change: 1 addition & 0 deletions src/v2/System/Router/Boot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { AnalyticsContext } from "../Analytics/AnalyticsContext"
import { ClientContext } from "v2/System/Router/buildClientAppContext"
import { SiftContainer } from "v2/Utils/SiftContainer"
import { setupSentryClient } from "lib/setupSentryClient"
import "v2/System/i18n/i18n"

export interface BootProps {
children: React.ReactNode
Expand Down
Loading