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

feat: Publish fabric and classic architecture release #1308

Merged
merged 117 commits into from
Feb 15, 2022

Conversation

Ubax
Copy link
Contributor

@Ubax Ubax commented Feb 9, 2022

Description

Checklist

  • Ensured that CI passes

Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

Good job! I added some comments to the code. Shouldn't we maybe move the fabric components to new folder in ios?

fontWeight: '700',
},
});
import {createNativeStackNavigator} from '@react-navigation/native-stack';
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to use v5 version of native-stack since it will get all the files straight from our repo? If not, could you check if we files imported from react-native-screens inside @react-navigation/native-stack are taken from our repo and not as some other version in its node_modules?

FabricExample/README.md Outdated Show resolved Hide resolved
FabricExample/README.md Outdated Show resolved Hide resolved
const styles = StyleSheet.create({
container: {
flex: 1,
paddingTop: Platform.OS === 'ios' ? 30 : 100,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this padding needed? So the options are visible when the header is translucent? Shouldn't it be managed by system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not managed by system if we have translucent off and header on

Copy link
Member

Choose a reason for hiding this comment

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

But then the content should be just below the header and the padding shouldn't be needed, am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I wanted to write is we have translucent on and header on
In this scenario the content will "hide" below the header

Copy link
Member

Choose a reason for hiding this comment

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

With ScrollView on iOS, the content should move itself to the correct position. On Android, it may be different though.

README-Fabric.md Outdated Show resolved Hide resolved
Comment on lines +120 to +126
#if defined(__IPHONE_OS_VERSION_MAX_ALLOWED) && defined(__IPHONE_13_0) && \
__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_13_0
if (@available(iOS 13.0, *)) {
// font customized on the navigation item level, so nothing to do here
} else
#endif
{
Copy link
Member

Choose a reason for hiding this comment

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

wdym by completely off?

src/fabric/ScreenNativeComponent.js Show resolved Hide resolved
@@ -91,8 +105,9 @@ let NativeFullWindowOverlay: React.ComponentType<View>;

const ScreensNativeModules = {
get NativeScreen() {
NativeScreenValue =
NativeScreenValue || requireNativeComponent('RNSScreen');
const nativeComponent = () =>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this code is wrapped in a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that it is not executed twice

Copy link
Member

Choose a reason for hiding this comment

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

So shouldn't we do it in current implementation too?

@Ubax Ubax force-pushed the @ubax/publish-fabric-classic branch from 08e5864 to eb3d555 Compare February 15, 2022 07:56
Base automatically changed from @ubax/fabric-example-app-3 to main February 15, 2022 10:06
@Ubax Ubax merged commit b22efc1 into main Feb 15, 2022
@Ubax Ubax deleted the @ubax/publish-fabric-classic branch February 15, 2022 11:01
tboba pushed a commit that referenced this pull request Apr 4, 2024
## Description

<!--
Description and motivation for this PR.

Include Fixes #<number> if this is fixing some issue.

Fixes # .
-->

Add missing `ScreenStackHeaderConfig` events to fabric spec and remove
the manual registration of events.

Also changed `Bubbling` events to `Direct` which fixes extraneous
lifecycle events. It was to be done quite a while ago already:
#1308 (comment)
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
## Description

<!--
Description and motivation for this PR.

Include Fixes #<number> if this is fixing some issue.

Fixes # .
-->

Add missing `ScreenStackHeaderConfig` events to fabric spec and remove
the manual registration of events.

Also changed `Bubbling` events to `Direct` which fixes extraneous
lifecycle events. It was to be done quite a while ago already:
software-mansion#1308 (comment)
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.

3 participants