Feat(suite): Enhance HiddenPlaceholder #13914
Conversation
| collectCoverage: true, | ||
| collectCoverageFrom: ['src/**/*.ts'], | ||
| testEnvironment: '../../JestCustomEnv.js', | ||
| }; |
There was a problem hiding this comment.
react-utils did not have unit tests at all. I set it up for the first test. Code coverage is not great 😄
| if (isValidElement(node)) { | ||
| const rewrittenChildren = rewriteReactNodeRecursively(node.props.children, callback); | ||
|
|
||
| return cloneElement(node, { ...node.props, key: reactKey, children: rewrittenChildren }); |
There was a problem hiding this comment.
TL;DR: Should I scrap this? 🤔
My idea was to have HiddenPlaceholder.tsx alone handle redacting numbers, so you would just wrap any content with it and it'd be completely automatic.
As I wrote in the comment there, this only works for simple cases of components wrapping children, such as:
<Wrapper>Label: <SomeStyle>12345 CZK</SomeStyle></Wrapper>.
But this procedure simply stops when it encounters more complex components, which calculate their content instead of just wrapping children, like <Formatter value={1234} />. They're returned "as is".
So those components have to do the redacting themselves: see prepareFiatAmountFormatter.ts, prepareCryptoAmountFormatter.ts.
I'm not happy with this mixed approach, where sometimes it does things automagically under the hood, but sometimes you just have to do it yourself. I fear this might lead to confusion and mistakes later on.
So should we drop this magic, and redact numbers at all specific places where we display them?
Reminder: this won't make HiddenPlaceholder.tsx useless, it still does CSS blur, and will provide the context for shouldRedactNumbers.
| @@ -34,11 +36,13 @@ export const HiddenPlaceholder = ({ | |||
| children, | |||
| enforceIntensity, | |||
| className, | |||
| ...rest | |||
| 'data-testid': dataTestId, | |||
There was a problem hiding this comment.
incidental cleanup, rest did not contain anything else
| if (Array.isArray(node)) { | ||
| return node.map((child, index) => { | ||
| const newReactKey: Key | undefined = isValidElement(child) | ||
| ? child.key ?? index |
There was a problem hiding this comment.
When rewriting the ReactNode, I do have to provide key even when the original element did not.
This is not an edge case, it's actually very common:
<span>label<i>123</i></span>
the span has children = array ["label", <i>123</i>]. Now the <i> has to have a key, idk why 🤦
|
|
||
| return formatter; | ||
| return FormatterComponent; |
There was a problem hiding this comment.
It is actually a React component, so its name should be in CamelCase.
Otherwise I get an unjustified error from eslint rule of hooks because of calling useShouldRedactNumbers.
| * @returns shouldRedactNumbers whether numbers should be redacted in displayed output | ||
| */ | ||
| export const useShouldRedactNumbers = () => | ||
| useContext(RedactNumbersContext)?.shouldRedactNumbers ?? false; |
There was a problem hiding this comment.
Not sure if all of this belongs to wallet-utils. Seemed suitable to me. Where would you place it?
|
Closing in favor of #14528 |
Discreet mode currently leaves numerical values blurred, but technically still possible to decypher.
Enhance it to actually replace the characters with
####, making them completely illegible.Description
TODO. WIP
########is wider than 0, it leads to horrible flickering if you hover at certain place#as per original length, but max 4Related Issue
Resolve #10567
Screenshots:
Uncovered numbers for reference:
Original, only blurred, but you can kinda discern the numbers:
Blured and redacted:
Blured and redacted onhover: