Fix Google Translate issues with React #1382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukaw3d
commented
Apr 4, 2023
Comment on lines
+58
to
+122
| // 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.', | ||
| }, |
There was a problem hiding this comment.
This probably covers most potential issues, but there is a possibility of undetectable:
const a = condition ? 'aa' : <span>bb</span>
return <Box>{a}</Box>There was a problem hiding this comment.
We should later make a package out of it, and reuse it on explorer
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports MegaLinter is graciously provided by |
|
snapshots |
Deploying with
|
| Latest commit: |
dbc2d1f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cdae65d4.oasis-wallet.pages.dev |
| Branch Preview URL: | https://lw-google-translate-crashes.oasis-wallet.pages.dev |
lukaw3d
force-pushed
the
lw/google-translate-crashes
branch
from
65c087f to
2121514
Compare
Codecov Report
@@ Coverage Diff @@
## master #1382 +/- ##
==========================================
- Coverage 84.32% 83.41% -0.91%
==========================================
Files 143 144 +1
Lines 3687 3732 +45
Branches 674 710 +36
==========================================
+ Hits 3109 3113 +4
- Misses 578 619 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
fixed |
buberdds
approved these changes
Apr 5, 2023
There was a problem hiding this comment.
-
src/app/pages/OpenWalletPage/Features/ImportAccountsSelectionModal/index.tsx
L~144 can we wrap {t('openWallet.importAccounts.pageNumber' with span so pagination counter is working when translation is on -
Would add
translate="no"to AmmountFormatter ticker ~L60
| <Language /> | ||
| </Box> | ||
| <Box pad="small" flex="grow"> | ||
| <Text>Language</Text> |
There was a problem hiding this comment.
I think we want to keep this one international. Otherwise if someone gets into French, they can't get out
lukaw3d
commented
Apr 5, 2023
| {!hideTicker && ( | ||
| <Text size={size} {...tickerProps}> | ||
| {' '} | ||
| {ticker} | ||
| <NoTranslate>{` ${ticker}`}</NoTranslate> |
There was a problem hiding this comment.
Would add translate="no" to AmmountFormatter ticker ~L60
Added, tho I found ticker translation cute
lukaw3d
commented
Apr 5, 2023
Comment on lines
+144
to
+145
| <span> | ||
| {t('openWallet.importAccounts.pageNumber', 'Page {{ pageNum }} of {{ totalPages }}', { |
There was a problem hiding this comment.
src/app/pages/OpenWalletPage/Features/ImportAccountsSelectionModal/index.tsx
L~144 can we wrap {t('openWallet.importAccounts.pageNumber' with span so pagination counter is working when translation is on
Added. Nice, it works. No idea why it works.
lukaw3d
force-pushed
the
lw/google-translate-crashes
branch
from
e1b1ae0 to
dbc2d1f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1354