Fix: Issues raised on iOS QA #325

tuliomir · 2023-09-11T15:33:42Z

Solves items from #318

Acceptance Criteria

Navigating to the "Receive" screen no longer marks an address as used
"Receive Amount" screen automatically focuses on the Keyboard
Clicking the "X" on the "Create Token" flow goes back to the dashboard screen
Clicking "Back" on the "Receive Transaction Details" after receiving a tx, goes back to the dashboard screen
Correct positioning of the "Receive Transaction Details" confirmation modal
Correct "Safe Area" insets for modals
Correct "Safe Area" insets for when the keyboard is being displayed
Dismisses keyboard whenever the "Pin Screen" is displayed

Tested on simulators for devices:

iPhone 13 mini
iPhone 13
iPhone SE (3rd gen)
iPhone 14 Pro

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

tuliomir · 2023-09-11T15:38:16Z

src/App.js

@@ -361,7 +361,6 @@ const AppStack = () => {
    >
      <Stack.Navigator
        screenOptions={{
-          presentation: 'modal',


Since the iOS modals exhibition have changed, this property is no longer desired.

andreabadesso · 2023-09-14T16:07:36Z

src/screens/CreateTokenAmount.js

@@ -114,7 +114,7 @@ class CreateTokenAmount extends React.Component {
        <HathorHeader
          title={t`CREATE TOKEN`}
          onBackPress={() => this.props.navigation.goBack()}
-          onCancel={() => this.props.navigation.pop()}
+          onCancel={() => this.props.navigation.getParent()?.goBack()}


What happens if there is no parent?

This ? operator was put there mostly as a coding best practice, but the idea is that there will always be a parent navigator there.

Not having a parent is a signal that there was a major refactor and our current hierarchy of navigators is different from expected. In that case, maybe throwing an exception would be a better way of informing the developer of the need to change this code as well.

Removed the optional chaining operator on a38a087 .

andreabadesso · 2023-09-14T16:09:45Z

src/screens/PaymentRequestDetail.js

-    await this.props.wallet.getNextAddress();
+    // When we create a new payment request, we don't update the address for a new one
+    // This will only happen when it receives a transaction and becomes a used address
+    await this.props.wallet.getCurrentAddress();


I don't understand what's going on here, did we change the logic for getCurrentAddress and getNextAddress? When?

This was requested on item n⁰ 9 on issue 318.

getNextAddress() not only fetches the next available address, but also marks the current one as used:

async getNextAddress() { // First we mark the current address as used, then return the next await this.getCurrentAddress({ markAsUsed: true });

By changing the call to getCurrentAddress() we don't do this marking, and leave it to the wallet's new transaction websocket handler to do so.

If there is no property, it's better to throw an error than to silence it.

pedroferreira1 · 2023-09-18T11:36:35Z

src/components/AmountTextInput.js

@@ -17,7 +17,13 @@ class AmountTextInput extends React.Component {
  }

  focus = () => {
-    this.inputRef.current.focus();
+    /* After the focus method is called, the screen is still re-rendered at least once more.


Why is it happening just now? It was working before, wasn't it?

Yes, it was working before.
I couldn't find a decisive answer for why this stopped working, but it probably has something to do with the general changes to the focus related functions from React Navigation 4 to 5.

A few considerations about this:

This focus command works perfectly when the screen is mounted at the same time it is rendered. So it works on the CreateTokenAmount and SendAmountInput just as it used to work before.

The problem happens on the NewPaymentRequest screen, that is built inside a tab bar. The AmountTextInput that needs to be focused here is mounted when the My Address tab is focused, and because of that is not available to be rendered/focused at mount time.

The tab management component currently used does not use the React Navigation material-top-tab-navigator wrapper, but instead uses directly its underlying react-native-tab-view lib.

That way, the focus event does not work as expected within the screen after the migration from Navigator 4 to 5 and 6. It only receives the focus event when changing from the Send stack to the Receive stack, for example.

I would suggest opening another PR to use the material-top-tab-navigator on this Receive stack instead of the tab view, so that we could have the focus events available on the screens again through React hooks and remove the edge case that brings the need for this setTimeout.

Ok. Create another issue to handle this and add it to the upgrade dependencies project, we can do it later

Opened issue

Automatic focus on input for Payment Request #331

pedroferreira1 · 2023-09-18T11:37:19Z

src/components/HathorHeader.js

@@ -75,7 +76,7 @@ const HathorHeader = (props) => {

 const styles = StyleSheet.create({
  wrapper: {
-    height: 56,
+    height: HEADER_HEIGHT,


The other PR you stop using this constant

That's right: while this PR was already under review I came up with a better organization for this on 326.
This variable will be short lived on the constants file.

pedroferreira1 · 2023-09-18T11:37:57Z

src/constants.js

+ * Default Hathor Header height, used for recalculating layout offsets.
+ * @type {number}
+ */
+export const HEADER_HEIGHT = 56;


This should be removed in the iOS color PR

The utils.js still uses it on the 326 color PR. It will be completely removed soon on the colors refactor:

refactor: string colors to themes #327

src/components/ReceiveMyAddress.js

src/utils.js

src/components/ModalConfirmation.js

src/screens/PaymentRequestDetail.js

tuliomir added 8 commits September 6, 2023 18:36

fix: consuming before receiving funds

fc5c75d

fix: AmountTextInput focus method

5c89094

fix: createToken navigation

3925140

fix: AppStack no longer modal

909dbf9

fix: back to dashboard on receive

08f8213

fix: improves keyboard avoiding distance

f531f5d

fix: modal bottom margin

78b04a1

fix: PinScreen incorrect keyboard

474d86e

tuliomir added the bug Something isn't working label Sep 11, 2023

tuliomir requested a review from alexruzenhack September 11, 2023 15:33

tuliomir requested a review from pedroferreira1 as a code owner September 11, 2023 15:33

tuliomir self-assigned this Sep 11, 2023

tuliomir commented Sep 11, 2023

View reviewed changes

style: removes unused variables

3da9db2

tuliomir requested review from andreabadesso and removed request for alexruzenhack September 11, 2023 15:49

andreabadesso reviewed Sep 14, 2023

View reviewed changes

refactor: removes optional chaining

a38a087

If there is no property, it's better to throw an error than to silence it.

tuliomir force-pushed the fix/ios-qa branch from d6dc13e to a38a087 Compare September 15, 2023 14:17

tuliomir requested a review from andreabadesso September 15, 2023 14:18

pedroferreira1 reviewed Sep 18, 2023

View reviewed changes

refactor: removes state

9b7fa36

tuliomir requested a review from pedroferreira1 September 18, 2023 16:02

fix: New Address button

803746c

tuliomir force-pushed the fix/ios-qa branch from e53e4b2 to 9302110 Compare September 18, 2023 21:49

fix: modal bottom margin

d610eb4

tuliomir force-pushed the fix/ios-qa branch from 9302110 to d610eb4 Compare September 18, 2023 23:22

pedroferreira1 approved these changes Sep 19, 2023

View reviewed changes

andreabadesso approved these changes Sep 19, 2023

View reviewed changes

tuliomir merged commit df914d5 into dev Sep 19, 2023
2 checks passed

tuliomir deleted the fix/ios-qa branch September 19, 2023 16:18

tuliomir mentioned this pull request Jan 9, 2024

Feat: Allow user to close numeric keyboard on iOS #407

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Issues raised on iOS QA #325

Fix: Issues raised on iOS QA #325

tuliomir commented Sep 11, 2023 •

edited

Loading

tuliomir Sep 11, 2023

andreabadesso Sep 14, 2023

tuliomir Sep 15, 2023

tuliomir Sep 15, 2023

andreabadesso Sep 14, 2023

tuliomir Sep 15, 2023 •

edited

Loading

pedroferreira1 Sep 18, 2023

tuliomir Sep 18, 2023

tuliomir Sep 18, 2023 •

edited

Loading

pedroferreira1 Sep 19, 2023

tuliomir Sep 19, 2023

pedroferreira1 Sep 18, 2023

tuliomir Sep 18, 2023

pedroferreira1 Sep 18, 2023

tuliomir Sep 18, 2023

Fix: Issues raised on iOS QA #325

Fix: Issues raised on iOS QA #325

Conversation

tuliomir commented Sep 11, 2023 • edited Loading

Acceptance Criteria

Security Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir commented Sep 11, 2023 •

edited

Loading

tuliomir Sep 15, 2023 •

edited

Loading

tuliomir Sep 18, 2023 •

edited

Loading