-
-
Notifications
You must be signed in to change notification settings - Fork 514
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: Screen props: Window traits (2) #1363
Conversation
statusBar*
statusBar*
(2)
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.
Looks good 🎉 I added some comments. Could you also expand FabricExample
so the newly added props are available to be tested there? Maybe something similar to Example
, where we have screens for different kinds of props.
@@ -378,6 +378,8 @@ - (void)invalidate | |||
|
|||
@end | |||
|
|||
#pragma mark - RNSScreen |
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.
Why is it here?
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.
Just for ease of navigation in file with implementations of 2+ classes, should I remove it?
@@ -31,6 +32,13 @@ - (instancetype)initWithFrame:(CGRect)frame | |||
static const auto defaultProps = std::make_shared<const RNSScreenProps>(); | |||
_props = defaultProps; | |||
_controller = [[RNSScreenController alloc] initWithView:self]; | |||
// TODO: use default props (?) |
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.
Yeah, I think we can leave just _gestureEnabled
@@ -194,10 +241,31 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const & | |||
const auto &oldScreenProps = *std::static_pointer_cast<const RNSScreenProps>(_props); | |||
const auto &newScreenProps = *std::static_pointer_cast<const RNSScreenProps>(props); | |||
|
|||
[super updateProps:props oldProps:oldProps]; | |||
[self setFullScreenSwipeEnabled:newScreenProps.fullScreenSwipeEnabled]; |
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.
Why do you sometimes just set the new prop and sometimes you check the old ones before setting new one?
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.
The props surrounded with if-statement have some additional logic in their setters, no reason to run it when the prop value is not changed.
android/src/paper See: #1328
ae55c29
to
92c1eef
Compare
This allows to share those types between old and new architecture
Required to enable status bar modifications
38a6ed2
to
310339c
Compare
statusBar*
(2)a6cca1d
to
9438df7
Compare
92c1eef
to
a8019a8
Compare
Description
Support screen props on Fabric
Part of stack PR:
fullScreenSwipeEnabled
(1) #1362screenOrientation
(3) #1364stackPresentation
&stackAnimation
(4) #1365Changes
Test code and steps to reproduce
status bar props
screen orientation
Checklist