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: make back button ripple visible #11386

Merged
merged 5 commits into from Aug 1, 2023
Merged

Conversation

vonovak
Copy link
Member

@vonovak vonovak commented May 27, 2023

Please provide enough information so that others can review your pull request.

Motivation

closes #9794

also observed that the ripple radius can be very slightly increased to be like in the native android toolbar

why the foreground isn't simply true? see facebook/react-native#37901

Test plan

tested locally

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (dcb4e4c) 75.48% compared to head (931bf2b) 75.49%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11386   +/-   ##
=======================================
  Coverage   75.48%   75.49%           
=======================================
  Files         190      190           
  Lines        5740     5742    +2     
  Branches     2260     2261    +1     
=======================================
+ Hits         4333     4335    +2     
  Misses       1360     1360           
  Partials       47       47           
Files Changed Coverage Δ
packages/elements/src/Header/HeaderBackButton.tsx 74.50% <100.00%> (+1.04%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vonovak vonovak requested a review from satya164 May 27, 2023 10:20
@netlify
Copy link

netlify bot commented May 27, 2023

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit 931bf2b
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/64c8bcf9d23a800008622827
😎 Deploy Preview https://deploy-preview-11386--react-navigation-example.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bberak
Copy link

bberak commented May 31, 2023

This should absolutely be merged.. It fixes a bug that's been outstanding since 2021.. #9794

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 15, 2023
Summary:
The Pressable component supports the `foreground` option for ripple. However, using it on old android api levels (e.g. android 5) crashes with

```
Error while updating property
nativeForegroundAndroid' of a view managed by:
RCTView
null
No virtual method setForeground(Landroid/ graphics/drawable/Drawable;)V in class Lcom/ facebook/react/views/view/ReactViewGroup; or its super classes
```

TouchableNativeFeedback.js has a check to make sure this does not happen [as seen here](https://github.com/facebook/react-native/blob/b0485bed0945061becace5af924fa60b17ab295f/packages/react-native/Libraries/Components/Touchable/TouchableNativeFeedback.js#L158)

I also want to point out that this PR can be closed #30466 as it's already implemented

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID] [FIXED] - foreground ripple crashed on api < 23

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #37901

Test Plan: tested locally on [another project](react-navigation/react-navigation#11386) because I wasn't able to get RN tester running

Reviewed By: rshest

Differential Revision: D46747310

Pulled By: NickGerleman

fbshipit-source-id: 4ee9062f821f5d629a1a0151c2fef61d836d09a4
@kacperkapusciak kacperkapusciak merged commit c43208f into main Aug 1, 2023
8 checks passed
@kacperkapusciak kacperkapusciak deleted the fix/back-button-ripple branch August 1, 2023 09:05
kacperkapusciak pushed a commit that referenced this pull request Aug 1, 2023
Please provide enough information so that others can review your pull
request.

**Motivation**

closes #9794

also observed that the ripple radius can be very slightly increased to
be like in the native android toolbar

why the `foreground` isn't simply `true`? see
facebook/react-native#37901

**Test plan**

tested locally
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.

Back button Android Ripple effect is not showing up on react-navigation 6
4 participants