-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
refactor: iOS Surface major refactoring
#3738
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
Conversation
|
Hey @DimitarNestorov, thank you for your pull request 🤗. The documentation from this branch can be viewed here. |
|
The mobile version of example app from this branch is ready! You can see it here. |
2b60477 to
8a8d924
Compare
Surface outer layerSurface styles
e3c4a2a to
485a239
Compare
Surface stylesSurface major refactoring
Surface major refactoringSurface major refactoring
This comment was marked as resolved.
This comment was marked as resolved.
| return ( | ||
| <Animated.View | ||
| ref={ref} | ||
| style={outerLayerViewStyles} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely, to get rid of the warning RCTView has a shadow set but cannot calculate shadow efficiently. I think the outer layer will need to have a backgroundColor style as well. However, it can require passing there also a borderRadius to sync it with the inner layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work but AnimatedFAB depends on backgroundColor to be transparent. I say we still do it and open a separate issue to figure something out for AnimatedFAB.
How to reproduce it on your branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to reproduce it on your branch?
I tried upgrading Expo but I didn't get warnings. Ended up creating an empty RN project and publishing Paper to npm: https://www.npmjs.com/package/@dimitarnestorov/react-native-paper/v/5.4.1-beta.0
Since transparent is hardcoded you need to go to node_modules/@dimitarnestorov/react-native-paper/src/components/FAB/AnimatedFAB.tsx and manually change the colour.
App.tsx:
import React from 'react';
import {
AnimatedFAB,
} from '@dimitarnestorov/react-native-paper';
function App(): JSX.Element {
return (
<AnimatedFAB
icon={'plus'}
label={'Label'}
extended={false}
visible
style={{right: 16, bottom: 16, position: 'absolute'}}
/>
);
}
export default App;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukewalczak Are you all planning fixing this warning for the AnimatedFAB? This PR resolved almost all of the warnings after upgrading to 5.5.0, with the exception of the AnimatedFAB. Is there any workaround for this at the moment? I can use patch-package until there is resolution on the matter.
485a239 to
5f99155
Compare
This comment was marked as resolved.
This comment was marked as resolved.
| }; | ||
| } | ||
|
|
||
| const SurfaceIOS = forwardRef< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a biggy but since we have MD2Surface MD3Surface SurfaceIOS and inlined logic for android and web - maybe we could extract each of them to a separate file
5f99155 to
019d96a
Compare
src/components/Surface.tsx
Outdated
| const [filteredStyle, marginStyle, borderRadiusStyle] = splitStyles( | ||
| restStyle, | ||
| (style) => style.startsWith('margin'), | ||
| (style) => style.startsWith('border') && style.endsWith('Radius') | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React Native team wants to deprecate StyleSheet.flatten so it's something we should keep in mind when writing new code and avoid such deep manipulation of styles.
019d96a to
b358ba0
Compare
…und with shadows
b358ba0 to
a919300
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested manually on the Android and web (on both MD generations) to confirm if there is no regression related to the previously fixed issue with different custom border radius values along with custom size - tests didn't present any problem!
Code was already reviewed by other contributors, comments were addressed.

Summary
Fixes #3733
Fixes #3593
Closes #3681
Changes in this PR:
Surfacework fine with percentage widths and heights on iOSSurfacefrom 3Animated.Views to 2SurfaceiOS logic in a separate componentAnimated.View:flexwidthheightopacitytranslatemarginsborderRadiuses andbackgroundColorto bothAnimated.ViewsAffected components:
AppbarBottomNavigationBarButtonCardChipAnimatedFABFABIconButtonMenuSurfaceSnackbarSearchbarModalBannerTest plan
Percentage width and height is applied correctly. Everything else looks as before.