See other comment

See other comment

See other comment

See other comment

The <Link> component needs a to prop, and should not be used for cases like this where you are triggering some sort of functionality rather than navigating somewhere. Replace with a <button>, or ideally a custom styled component button.

Not sure why the removal of <SettingsLinkCtn> is part of this diff...is this not solved by a rebase?

Was this container removed as part of this diff?

I am seeing it in the latest master. Why was this removed?

Why null and not false?

Above, you have the app displaying something {!cashtabSettings.showMessages &&

Here, you have the app displaying something {cashtabSettings.showMessages && (

It will always be one or the other. So, it is better practice to use conditional rendering like

`{cashtabSettings.showMessages ? <>the thing to show if it's true<> : <>the thing to show if it's false<>}

This should be passed into the Tx component as a prop, because it does not differ for each tx.

1. In Home.js, pass cashtabSettings as a prop to <TxHistory>
2. In TxHistory.js, pass cashtabSettings as a prop to <Tx>
3. Remove the WalletContext import in Tx.js and use the prop

Seems like a confusing boolean. The boolean showMessages, if true, creates a condition where messages are hidden by default?

Am I reading this right?

If you are saying "if this, display a bunch of components, otherwise display an empty string"

needs to be a good reason to just display the empty string. Otherwise, use

{data.opReturnMessage && !data.isEncryptedMessage && (

Maybe I misinterpreted the code when I earlier asked to use the ? : conditional style. It should always be used if there are 2 cases and you are displaying one thing or the other.

If you are displaying something or nothing, use &&

see above

The presence or absence of this <UnauthorizedDecryptionMessage> component seems to be the only difference in code between

{/*encrypted and wallet is authorized to view OP_RETURN Message*/}

and

{/*encrypted but wallet is not authorized to view OP_RETURN Message*/}

Yet all of this other surrounding stuff is duplicated.

Conditional rendering should be limited only to what is necessary. This also makes the code easier to read -- it's quite difficult to review this code and find out exactly what is changing depending on the conditions.

In this case, you can have conditional rendering inside this component, either showing

<p>
 {data.opReturnMessage}
</p>

or

<UnauthorizedDecryptionMessage>
{data.opReturnMessage}
</UnauthorizedDecryptionMessage>

depending on data.decryptionSuccess

Another, probably better approach would be to have a styled component <DecryptedMsg> that accepts authorized={data.decryptionSuccess} as a boolean prop. Then add a styled component rule to change the styling...conventional <p> if authorized={true} and <UnauthorizedDecryptionMessage> otherwise.

There are some other styled components in Cashtab that use this type of logic.

You should always be looking to write the absolute minimum code required to implement a change.

note that cashtabSettings is a boolean (false) when the app first loads, as loading them from localstorage is async.

Proptypes should reflect this

Again, need to always run with the dev console open and check for these types of msgs:

same as above