LIVE-4370 tron more explicit error #1962
Conversation
π¦ Changeset detectedLatest commit: a5223e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
desktop
|
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
apps/ledger-live-desktop/src/renderer/components/TranslatedError.jsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/families/tron/StepReceiveFunds.jsx
Show resolved
Hide resolved
| hideToast(); | ||
| }, [setIsAddionalInfoModalOpen, hideToast]); | ||
|
|
||
| const closeAdditionalInfoModal = useCallback(() => { |
There was a problem hiding this comment.
We should be more careful with how we call useCallback, useMemo and such as it's sometimes more costly in performance to call it than not. It's not necessary here
There was a problem hiding this comment.
It's just a copy paste of an existing file, I duplicate it because we want this error to be exclusive to tron, I have no idea what most of contains of the file is
There was a problem hiding this comment.
@lvndry I'm interested with this argument about performance, do you have details about it?
From what we've read, the benefit of using hooks all the time without thinking about it is mostly superior to the cost, cf. this explanation for instance: https://attardi.org/why-we-memo-all-the-things/
If you have doc stating otherwise feel free to share.
There was a problem hiding this comment.
@haammar-ledger Yep this could be discussed outside of here but IMO hooks are tools to optimise (meaning making fast code, faster) and not a tool that improves performance (making slow code, fast).
My arguments against overusing hooks are:
- Most of the time perfomance gains of
useCallbackoruseMemoare basically unnoticeable but we still do function calls which impact our perf - More verbose codebase
- This leads to the wrong idea that these hooks solve bigger performance issues, like thinking that as long as we wrap everything with
useCallback,useMemowe don't have to care about things that actually have bigger like useless state change, useless rerenders, component composition, etc - The react doc states that useMemo might not use the memoized values everytime, it's not a guarantee so we should not rely on them by default
Here are some interesting blog posts:
https://kentcdodds.com/blog/usememo-and-usecallback
https://www.joshwcomeau.com/react/usememo-and-usecallback/
https://www.developerway.com/posts/how-to-use-memo-use-callback
https://maxrozen.com/understanding-when-use-usememo
apps/ledger-live-desktop/src/renderer/families/tron/StepReceiveFunds.jsx
Show resolved
Hide resolved
| /> | ||
| {verifyAddressError ? ( | ||
| <ErrorDisplay error={verifyAddressError} onRetry={onVerify} /> | ||
| ) : isAddressVerified === true ? ( |
There was a problem hiding this comment.
The typing of isAddressVerified is kinda weird if we have to check true with strict equality π€
Same thing for the isAddressVerified === false just under.
I know .jsx is a pain in the ass for simple if/elseif/else, but all of those imbricated ternary operators are making me dizzy π
{
verifyAddressError ? (
A
) : isAddressVerified === true ? (
B
) : isAddressVerified === false ? (
C
) : device ? (
D
) : null // should not happen <--- wut
}And B & C are actually containing ternaries as well π€―
Again nothing mandatory, but that would probably help to have a simple function with if/elseif/else outside of the templating imo.
There was a problem hiding this comment.
Weird stuff indeed. But yeah I'm too afraid at this point to change a thing on this file since most of the code doesn't belongs to me.
I can try if I have not other things to fix tho
|
@henrily-ledger Screenshots: β
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
| ) : null; | ||
|
|
||
| return ( | ||
| <Text> |
There was a problem hiding this comment.
Being extra careful here, but do you think <Text> can never change the display, depending on the context? I'm just wondering because of how TranslatedComponent is used in so many places...
There was a problem hiding this comment.
I don't think it can but we maybe need to be careful about this
There was a problem hiding this comment.
I'll try to do an extra condition for not impacting unless we got a learn more
Codecov ReportBase: 42.85% // Head: 42.85% // No change to project coverage π
Additional details and impacted files@@ Coverage Diff @@
## develop #1962 +/- ##
========================================
Coverage 42.85% 42.85%
========================================
Files 624 624
Lines 25895 25895
Branches 7159 7159
========================================
Hits 11098 11098
Misses 13640 13640
Partials 1157 1157
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. β View full report at Codecov. |
β¦n-errors-trc-20
π Description
Add learn more link to error message + add more explicit error for tron
β Context
live-mobile, live-desktopβ Checklist
πΈ Demo
π Expectations to reach
Please make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.