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

Jump to bottom

frontend: transform aopp.tsx to funcitonal component #2761

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

frontend: transform aopp.tsx to funcitonal component #2761

wants to merge 1 commit into from

Conversation

NicolaLS
Copy link
Contributor

@NicolaLS NicolaLS commented Jun 11, 2024 •
edited
Loading

Tested by patching the Start function in backend like this.

I actually forgot to remove the commit, but I guess it is good to leave it in there for reviewers to test the PR. I can remove it in the end. (note the delay is 15 seconds when testing). removed it now.

I tested on webdev firefox with BB02B keystore and servewallet-mainnet. I tested the following case statements form the return: error inactive user-approval awaiting-keystore signing

I think choosing-account and syncing was tested too and looked fine but it was to fast to take a screenshot.

I think I did not test success because I don't know a callback to test aopp with.

Some pictures from testing:

aopp-test4 png
aopp-test3
aopp-test2
test-aopp1

@NicolaLS NicolaLS marked this pull request as draft June 11, 2024 00:32
@NicolaLS NicolaLS force-pushed the refactor-functional-aopp branch 2 times, most recently from 855fcd3 to a0bbec8 Compare June 11, 2024 00:40
@NicolaLS NicolaLS marked this pull request as ready for review June 11, 2024 00:46
thisconnect
thisconnect requested changes Jun 13, 2024
View reviewed changes
frontends/web/src/components/aopp/aopp.tsx Outdated Show resolved Hide resolved
frontends/web/src/components/aopp/aopp.tsx Outdated Show resolved Hide resolved
frontends/web/src/components/aopp/aopp.tsx Outdated Show resolved Hide resolved
@NicolaLS NicolaLS force-pushed the refactor-functional-aopp branch 2 times, most recently from c86fd7c to 130d760 Compare June 13, 2024 16:23
@NicolaLS
Copy link
Contributor Author

@thisconnect thanks :) PTAL, I added a comment regarding the shouldUpdateAccountDefault explaining the reasoning.

@NicolaLS NicolaLS force-pushed the refactor-functional-aopp branch 4 times, most recently from c09a10d to 7945bc3 Compare June 17, 2024 16:10
thisconnect
thisconnect requested changes Jun 18, 2024
View reviewed changes
frontends/web/src/components/aopp/aopp.tsx Outdated Show resolved Hide resolved
frontends/web/src/components/aopp/aopp.tsx Outdated Show resolved Hide resolved
@NicolaLS
Copy link
Contributor Author

@thisconnect followed your suggestions and removed the shouldUpdateAccountDefault stuff, and tested again.

@NicolaLS
Copy link
Contributor Author

NicolaLS commented Jul 8, 2024

not sure why CI fails, running make ci locally succeeds

@thisconnect
Copy link
Collaborator

not sure why CI fails, running make ci locally succeeds

not related to your PR, we have a new script that checks for missing placeholders in translations 3d34cb6
and also Ci does localize check

bitbox-wallet-app/scripts/ci.sh

Line 35 in 2a2e2b8

if ! locize format frontends/web/src/locales --format json --dry true ; then

master was failing for a while as some placeholders in translations were missing, but it should pass since #2794

@benma
Copy link
Contributor

benma commented Jul 9, 2024

@thisconnect actually it was a different error, some transient backend/handler test failed for some reason. Re-run was successful.

@thisconnect
Copy link
Collaborator

actually it was a different error, some transient backend/handler test failed for some reason. Re-run was successful.

oups I just assumed and didn't check 😇 sorry

thisconnect
thisconnect requested changes Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@thisconnect thisconnect left a comment

Choose a reason for hiding this comment

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

cc @shonsirsha can you take this review?

frontends/web/src/components/aopp/aopp.tsx Outdated
this.setState(currentState => {
if (aopp?.state === 'choosing-account'
useEffect(() => {
setAccountCodeDefault(aopp, setAccountCode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

looking at the git diff setAccountCodeDefault is confusing me a bit and seems to work differently now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I defined it outside of the component so aopp and setAccountCode need to be passed to it but it is essentially the same: aopp comes from the state so same as this. and setAccountCode sets the accountCode state so same as this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can put it back into the component so it does not need any arguments, I just like the functional style more for some reason but I think it does not really have any impact on anything here (e.g. performance)

@shonsirsha
Copy link
Collaborator

shonsirsha commented Aug 21, 2024 •
edited
Loading

cc @shonsirsha can you take this review?

Sorry just saw this! Yes, I can. 👍

@NicolaLS , feel free to tag me as a reviwer once it's ready to be re-reviewed 😇 ty

@NicolaLS
Copy link
Contributor Author

cc @shonsirsha can you take this review?

Sorry just saw this! Yes, I can. 👍

@NicolaLS , feel free to tag me as a reviwer once it's ready to be re-reviewed 😇 ty

@shonsirsha Thanks, yes it is ready (just rebased)

@shonsirsha shonsirsha self-requested a review September 2, 2024 08:15
shonsirsha
shonsirsha requested changes Sep 3, 2024
View reviewed changes
Copy link
Collaborator

@shonsirsha shonsirsha left a comment

Choose a reason for hiding this comment

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

Looks good, just a comment on the error message.

Tested on Pocket Bitcoin (QT) and seems to work:

pb pb2 pb3

frontends/web/src/components/aopp/aopp.tsx Outdated
<Message type="error">
<Cancel className={styles.smallIcon} />
{t(`error.${aopp.errorCode}`, { host: domain(aopp.callback) })}
</Message>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks strange now with our new Message component. I think we can remove the Cancel icon (and remove it from codebase too if it's totally unused).

looks weird?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the Cancel and now it looks like this:

image

Did you mean it is totally unused in the frontend or totally unused in the component ? I'll check if it can be removed globally and do that in a separate commit then.

frontends/web/src/components/aopp/aopp.tsx Outdated
}

type Props = TranslateProps;
const setAccountCodeDefault = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see a strong reason why we want to have this outside of the component. It doesn't seem to follow usual code pattern (setting state outside of the component itself).

The Aopp component also gets a lot shorter with this refactor - and having this fn in the component doesn't seem bad at all? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, I generally like a more functional approach so the dependencies are clear, also technically this improves performance because the function is not defined/initialized on every render but performance does not really matter here xd.

I'm going to put it back in the component

@NicolaLS
Copy link
Contributor Author

NicolaLS commented Sep 4, 2024

(addressed change requests in interactive rebase, might add a commit to remove Cancel globally now if possible)

@NicolaLS
Copy link
Contributor Author

NicolaLS commented Sep 4, 2024

@shonsirsha thanks, I addressed everything PTAL.

This looks strange now with our new Message component. I think we can remove the Cancel icon (and remove it from codebase too if it's totally unused).

I searched for Cancel and it is used in KeystoreConnectPrompt in the switch(data.typ) ...case 'error': .. so I don't think we can remove it. It is also used in IconAndMessage <- MessageWaitDialog <- Send

shonsirsha
shonsirsha approved these changes Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@shonsirsha shonsirsha left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisconnect thisconnect thisconnect requested changes

@shonsirsha shonsirsha shonsirsha approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NicolaLS @thisconnect @benma @shonsirsha