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.

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

fix: the NFT reward bottom sheet unexpectedly reappears after CTA button is pressed #5076

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

bakoushin
Copy link
Contributor

@bakoushin bakoushin commented Mar 12, 2024

Description

On the iOS production build the NFT reward bottom sheet unexpectedly reappears after CTA button is pressed.
The hypothesis is that onChange event isn't fired for some reason when the bottom sheet is closed programmatically.

The fix is to fire dispatch on CTA press handler instead of relying on onChange event.

Another possible option is to rely on onClose event. But unfortunately it is called when component unmounts on Android.

Test plan

Tested manually on:

  • Android emulator
  • iOS simulator
  • dev iOS build on device

Planing to additionally test on the nightly iOS build.

Related issues

Backwards compatibility

Y

Network scalability

NA

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 85.58%. Comparing base (a658959) to head (c042cd6).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5076   +/-   ##
=======================================
  Coverage   85.57%   85.58%           
=======================================
  Files         724      724           
  Lines       29507    29508    +1     
  Branches     5100     5100           
=======================================
+ Hits        25252    25253    +1     
  Misses       4021     4021           
  Partials      234      234           
Files Coverage Δ
src/home/celebration/NftReward.tsx 88.05% <75.00%> (+0.18%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a658959...c042cd6. Read the comment docs.

@bakoushin bakoushin marked this pull request as ready for review March 12, 2024 12:57
Copy link

emerge-tools bot commented Mar 12, 2024

1 build decreased size

Name Version Download Change Install Change Approval
Celo (test)
org.celo.mobile.test
1.80.0 (145) 24.2 MB ⬇️ 2.4 MB (-8.94%) 60.4 MB ⬇️ 2.7 MB (-4.27%) N/A

Celo (test) 1.80.0 (145)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬇️ 2.7 MB (-4.27%)
Total download size change: ⬇️ 2.4 MB (-8.94%)

Largest size changes

Item Install Size Change
🗑 splashBackground@3x.jpg ⬇️ -600.2 kB
🗑 background@3x.jpg ⬇️ -368.6 kB
🗑 boost-rewards@3x.png ⬇️ -188.4 kB
🗑 background@2x.jpg ⬇️ -176.1 kB
🗑 boost-rewards@2x.png ⬇️ -90.1 kB
View Treemap

Image of diff


🛸 Powered by Emerge Tools


dispatch(nftRewardDisplayed())
dispatch(nftRewardDisplayed())
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be easily tested? looks like patch coverage is failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately I don't know the way to simulate bottom sheet close.
the only way I see is to trick coverage bot for now.

@bakoushin bakoushin added this pull request to the merge queue Mar 12, 2024
Merged via the queue into main with commit 4d484cf Mar 12, 2024
16 checks passed
@bakoushin bakoushin deleted the alex/fix-nft-reward-bottom-sheet branch March 12, 2024 17:45
kathaypacific pushed a commit that referenced this pull request Mar 13, 2024
…ton is pressed (#5076)

### Description

On the iOS production build the NFT reward bottom sheet unexpectedly
reappears after CTA button is pressed.
The hypothesis is that `onChange` event isn't fired for some reason when
the bottom sheet is closed programmatically.

The fix is to fire `dispatch` on CTA press handler instead of relying on
`onChange` event.

Another possible option is to rely on `onClose` event. But unfortunately
it is called when component unmounts on Android.

### Test plan

Tested manually on:
* Android emulator
* iOS simulator
* dev iOS build on device

Planing to additionally test on the nightly iOS build.

### Related issues

- Related to  RET-1002

### Backwards compatibility

Y

### Network scalability

NA
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
…ton is pressed (valora-inc#5076)

### Description

On the iOS production build the NFT reward bottom sheet unexpectedly
reappears after CTA button is pressed.
The hypothesis is that `onChange` event isn't fired for some reason when
the bottom sheet is closed programmatically.

The fix is to fire `dispatch` on CTA press handler instead of relying on
`onChange` event.

Another possible option is to rely on `onClose` event. But unfortunately
it is called when component unmounts on Android.

### Test plan

Tested manually on:
* Android emulator
* iOS simulator
* dev iOS build on device

Planing to additionally test on the nightly iOS build.

### Related issues

- Related to  RET-1002

### Backwards compatibility

Y

### Network scalability

NA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants