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

Fix Google Translate issues with React #1382

Merged
merged 9 commits into from
Apr 5, 2023
Merged

Fix Google Translate issues with React #1382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
13285b8
Fix Google Translate issues with React in i18n Trans
lukaw3d Apr 3, 2023
a599e99
Detect Google Translate issues with React with ESLint
lukaw3d Apr 3, 2023
ffeb0b9
Ignore detected fragments made of elements
lukaw3d Apr 4, 2023
2352906
Fix detected Google Translate issues with React
lukaw3d Apr 3, 2023
2e3359f
Ignore false positives
lukaw3d Apr 4, 2023
d6862e2
Don't translate ticker name
lukaw3d Apr 5, 2023
f54f1b9
Comment language switcher intentionally isn't i18n
lukaw3d Apr 5, 2023
6ea9dfd
Fix Google Translate updating page numbers by wrapping it into span
lukaw3d Apr 5, 2023
dbc2d1f
Update jest snapshots
lukaw3d Apr 4, 2023
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
72 changes: 72 additions & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ const config = {
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-target-blank.md#custom-link-components
linkComponents: ['Button', 'MediaButton', 'SidebarButton', 'Anchor', 'Link', 'AnchorLink'],
},
reportUnusedDisableDirectives: true,

rules: {
'no-restricted-imports': [
'error',
Expand Down Expand Up @@ -49,6 +51,76 @@ const config = {
},
],
'prefer-template': 'error',
'no-restricted-syntax': [
// Find google translate issues: https://github.com/facebook/react/issues/11538#issuecomment-390386520
'error',

// Ban `condition && text node`
{
selector:
'JSXElement > JSXExpressionContainer > LogicalExpression[operator="&&"]' +
'[right.type!="JSXElement"][right.type!="JSXFragment"]',
message:
'Conditional plain text nodes could break React if used with Google Translate. Wrap text into an element.',
},
// Ban `condition && <>text node</>`
{
selector:
'JSXElement > JSXExpressionContainer > LogicalExpression[operator="&&"]' +
' > JSXFragment > .children:not(JSXElement, JSXText[value=/^\\s+$/])',
message:
'Conditional plain text nodes could break React if used with Google Translate. Wrap text into an element.',
},
// Ban text nodes before or after a condition `text {condition && <span/>} text`
{
selector:
'JSXElement:has(JSXExpressionContainer.children > LogicalExpression[operator="&&"])' +
' > JSXText[value!=/^\\s+$/]',
message:
'Plain text nodes before or after a condition could break React if used with Google Translate. Wrap text into an element.',
},
// Ban variables before or after `{var} {condition && <span/>} {var}` (just in case they return a string)
{
selector:
'JSXElement:has(JSXExpressionContainer.children > LogicalExpression[operator="&&"])' +
' > JSXExpressionContainer:matches([expression.type="Identifier"], [expression.type="CallExpression"])',
message:
'Plain text nodes before or after a condition could break React if used with Google Translate. Identifier could possibly return a string, so wrap it into an element.',
},

// Ban `condition ? text node : <span/>`
{
selector:
'JSXElement > JSXExpressionContainer > ConditionalExpression' +
' > :matches(.consequent, .alternate):not(JSXElement, JSXFragment)',
message:
'Conditional plain text nodes could break React if used with Google Translate. Wrap text into an element.',
},
// Ban `condition ? <>text node</> : <span/>`
{
selector:
'JSXElement > JSXExpressionContainer > ConditionalExpression' +
' > JSXFragment > .children:not(JSXElement, JSXText[value=/^\\s+$/])',
message:
'Conditional plain text nodes could break React if used with Google Translate. Wrap text into an element.',
},
// Ban text nodes before or after a condition `text {condition ? <span/> : <span/>} text`
{
selector:
'JSXElement:has(JSXExpressionContainer.children > ConditionalExpression)' +
' > JSXText[value!=/^\\s+$/]',
message:
'Plain text nodes before or after a condition could break React if used with Google Translate. Wrap text into an element.',
},
// Ban variables before or after `{var} {condition ? <span/> : <span/>} {var}` (just in case they return a string)
{
selector:
'JSXElement:has(JSXExpressionContainer.children > ConditionalExpression)' +
' > JSXExpressionContainer:matches([expression.type="Identifier"], [expression.type="CallExpression"])',
message:
'Plain text nodes before or after a condition could break React if used with Google Translate. Identifier could possibly return a string, so wrap it into an element.',
},
Comment on lines +58 to +122
Copy link
Member Author

Choose a reason for hiding this comment

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

This probably covers most potential issues, but there is a possibility of undetectable:

const a = condition ? 'aa' : <span>bb</span>
return <Box>{a}</Box>

Copy link
Member Author

Choose a reason for hiding this comment

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

We should later make a package out of it, and reuse it on explorer

],

'react/jsx-no-target-blank': ['error', { allowReferrer: true }],
'react/react-in-jsx-scope': 'off', // Not needed after React v17
Expand Down
24 changes: 18 additions & 6 deletions src/app/components/AmountFormatter/__tests__/__snapshots__/index.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,18 @@ exports[`<AmountFormatter /> should render component 1`] = `

<div>
<span>
456.542341274
<span>
456.542341274
</span>
<span
class="c0"
>

ROSE
<span
class="notranslate"
translate="no"
>
ROSE
</span>
</span>
</span>
</div>
Expand All @@ -29,12 +35,18 @@ exports[`<AmountFormatter /> should render component with small ticker 1`] = `

<div>
<span>
456.542341274
<span>
456.542341274
</span>
<span
class="c0"
>

ROSE
<span
class="notranslate"
translate="no"
>
ROSE
</span>
</span>
</span>
</div>
Expand Down
8 changes: 4 additions & 4 deletions src/app/components/AmountFormatter/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { useSelector } from 'react-redux'
import { Text } from 'grommet'
import { StringifiedBigInt } from 'types/StringifiedBigInt'
import { formatBaseUnitsAsRose, formatWeiAsWrose } from 'app/lib/helpers'
import { NoTranslate } from '../NoTranslate'

export interface AmountFormatterProps {
amount: StringifiedBigInt | null
Expand All @@ -36,7 +37,7 @@ export const AmountFormatter = memo(
}: AmountFormatterProps) => {
const ticker = useSelector(selectTicker)
const isUsingBaseUnits = amountUnit === 'baseUnits'
if (amount == null) return <>-</>
if (amount == null) return <span>-</span>

const formatter = isUsingBaseUnits ? formatBaseUnitsAsRose : formatWeiAsWrose
const amountString = formatter(amount, {
Expand All @@ -55,11 +56,10 @@ export const AmountFormatter = memo(

return (
<span>
{amountString}
<span>{amountString}</span>
{!hideTicker && (
<Text size={size} {...tickerProps}>
{' '}
{ticker}
<NoTranslate>{` ${ticker}`}</NoTranslate>
Copy link
Member Author

Choose a reason for hiding this comment

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

Would add translate="no" to AmmountFormatter ticker ~L60

Added, tho I found ticker translation cute

</Text>
)}
</span>
Expand Down
28 changes: 21 additions & 7 deletions src/app/components/Footer/__tests__/__snapshots__/index.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,16 +222,22 @@ exports[`<Footer /> should render mobile version of footer 1`] = `
<span
class="c1"
>
Oasis Wallet is fully
<span>
Oasis Wallet is fully
</span>
<a
class="c2"
href="https://github.com/oasisprotocol/oasis-wallet-web/"
rel="noopener noreferrer"
target="_blank"
>
open source
<span>
open source
</span>
</a>
- Feedback and issues are appreciated!
<span>
- Feedback and issues are appreciated!
</span>
</span>
<span
class="c1"
Expand All @@ -242,13 +248,17 @@ exports[`<Footer /> should render mobile version of footer 1`] = `
rel="noopener noreferrer"
target="_blank"
>
Terms and Conditions
<span>
Terms and Conditions
</span>
</a>
</span>
<span
class="c3"
>
Version:
<span>
Version:
</span>
<a
class="c2"
href="https://github.com/oasisprotocol/oasis-wallet-web/releases/tag/v1.0.0-dev.1"
Expand All @@ -257,7 +267,9 @@ exports[`<Footer /> should render mobile version of footer 1`] = `
>
1.0.0-dev.1
</a>
(commit:
<span>
(commit:
</span>
<a
class="c2"
href="https://github.com/oasisprotocol/oasis-wallet-web/commit/sha0000000000000000000000000000000000000"
Expand All @@ -266,7 +278,9 @@ exports[`<Footer /> should render mobile version of footer 1`] = `
>
sha0000
</a>
) built at 1/1/1970, 12:00:00 AM
<span>
) built at 1/1/1970, 12:00:00 AM
</span>
<div
class="c4"
>
Expand Down
29 changes: 17 additions & 12 deletions src/app/components/Sidebar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ export const SidebarButton = ({
gap="medium"
justify={isMediumSize ? 'center' : 'start'}
>
{/* eslint-disable-next-line no-restricted-syntax -- icon is not a plain text node */}
{icon}
{!isMediumSize && <Text>{label}</Text>}
</Box>
Expand Down Expand Up @@ -224,21 +225,24 @@ const SidebarFooter = (props: SidebarFooterProps) => {
dropProps={{ align: { bottom: 'bottom', left: 'left' } }}
items={languageLabels.map(([key, label]) => ({ label: label, onClick: () => setLanguage(key) }))}
>
<Box direction="row">
{size === 'medium' ? (
<Box pad="small">
<Language />
</Box>
{size !== 'medium' && (
<>
<Box pad="small" flex="grow">
<Text>Language</Text>
</Box>
<Box pad="small">
<FormDown />
</Box>
</>
)}
</Box>
) : (
<Box direction="row">
<Box pad="small">
<Language />
</Box>
<Box pad="small" flex="grow">
{/* Intentionally not translated */}
<Text>Language</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

missing i18n label

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we want to keep this one international. Otherwise if someone gets into French, they can't get out

</Box>
<Box pad="small">
<FormDown />
</Box>
</Box>
)}
</Menu>
</Box>
</SidebarTooltip>
Expand Down Expand Up @@ -293,6 +297,7 @@ function SidebarMenuItems() {
{menu.home}
{menu.wallet}
{menu.stake}
{/* eslint-disable-next-line no-restricted-syntax -- menu.paraTimes is not a plain text node */}
{canAccessParaTimesRoute && menu.paraTimes}
</Nav>
)
Expand Down
11 changes: 9 additions & 2 deletions ...components/Toolbar/Features/AccountSelector/__tests__/__snapshots__/Account.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,11 +412,18 @@ exports[`<Account /> should match snapshot 1`] = `
class="c15"
>
<span>
0.000001
<span>
0.000001
</span>
<span
class="c16"
>

<span
class="notranslate"
translate="no"
>

</span>
</span>
</span>
</div>
Expand Down
11 changes: 9 additions & 2 deletions ...p/components/Toolbar/Features/AccountSelector/__tests__/__snapshots__/index.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,11 +510,18 @@ exports[`<AccountSelector /> should match snapshot 1`] = `
class="c14"
>
<span>
0.0000001
<span>
0.0000001
</span>
<span
class="c15"
>

<span
class="notranslate"
translate="no"
>

</span>
</span>
</span>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/app/components/Transaction/InfoBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export function InfoBox({ copyToClipboard, icon: IconComponent, label, trimValue

<Box justify="center">
<Text weight="bold">{label}</Text>
<Text>{trimValue ? trimLongString(value) : value}</Text>
{trimValue ? <Text>{trimLongString(value)}</Text> : <Text>{value}</Text>}
</Box>
{notificationVisible && (
<Notification
Expand Down
32 changes: 24 additions & 8 deletions src/app/components/Transaction/__tests__/__snapshots__/index.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,19 +608,27 @@ exports[`<Transaction /> should handle unknown transaction types gracefully 1`]
class="c16"
>
<span>
0.001
<span>
0.001
</span>
<span
class="c17"
>

TEST
<span
class="notranslate"
translate="no"
>
TEST
</span>
</span>
</span>
</span>
<span
class="c18"
>
account.transaction.successful
<span>
account.transaction.successful
</span>
</span>
</div>
</div>
Expand Down Expand Up @@ -1264,19 +1272,27 @@ exports[`<Transaction /> should match snapshot 1`] = `
class="c16"
>
<span>
0.001
<span>
0.001
</span>
<span
class="c17"
>

TEST
<span
class="notranslate"
translate="no"
>
TEST
</span>
</span>
</span>
</span>
<span
class="c18"
>
account.transaction.successful
<span>
account.transaction.successful
</span>
</span>
</div>
</div>
Expand Down
Loading