Skip to content

Conversation

@Gregoirevda
Copy link
Contributor

Summary:

Adding equivalent of existing DrawerLayoutAndroid.
I'm using the method added in PR #37873 for AppDelegate too only contain enhancements.
Native Drawer, with gestures and button to open and close.
It does have issues:

  • Can't make flex-1 work within DrawerLayoutIos children. DrawerLayoutIos needs to have a specific width and height. Can't bound it to the drawer full size.
  • Children of DrawerLayoutIos are correctly rendered and updated in the drawer, but they are also rendered within the current tree
Screenshot 2023-06-15 at 23 50 46

Changelog:

Pick one each for the category and type tags:

[IOS] [ADDED] - Adding LayoutDrawerIos

Test Plan:

Tested on simulator iPad Pro iOS 16.2

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 16, 2023
@analysis-bot
Copy link

analysis-bot commented Jun 16, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,044,767 -512
android hermes armeabi-v7a 8,294,048 -498
android hermes x86 9,560,950 -512
android hermes x86_64 9,403,301 -502
android jsc arm64-v8a 9,603,752 -779
android jsc armeabi-v7a 8,730,474 -769
android jsc x86 9,690,805 -766
android jsc x86_64 9,937,143 -769

Base commit: 637ffb1
Branch: main

@Gregoirevda
Copy link
Contributor Author

To the reviewer, this is definitely WIP. I think we should have DrawerLayoutIos props aligned with DrawerLayoutAndroid and have the native code in Libraries instead of template. Which isn't too much work. I would like to get a sense of the feasibility to have this component added in RN first.

How can I easily edit Library instead and have it running locally in template or another app?

@Gregoirevda Gregoirevda changed the title feat: LayoutDrawerIos feat: DrawerLayoutIos Jun 16, 2023
@Pranav-yadav
Copy link
Contributor

Just for ref: this is re-land of #37924

@Pranav-yadav Pranav-yadav added Type: Enhancement A new feature or enhancement of an existing feature. Needs: Docs Website PR To encourage the authors to create a website PR for documentation. labels Jun 17, 2023
@Pranav-yadav
Copy link
Contributor

@cortinico can you ping someone to review this?

@Gregoirevda
Copy link
Contributor Author

Gregoirevda commented Jun 28, 2023

@Pranav-yadav I started the refactor yesterday evening. I'm updating react views instead of the template and using the drawer in rn-tester app as it should be done. I'll also align the interface between both; especially the renderDrawer prop.

There will be a new prop introduced 'visible' which would be responsible from Js to have the drawer view setup (open and close methods still responsible for showing it). As currently DrawerLayoutAndroid doesn't work with react navigation because a view doesn't unmount when navigating away. Fixes #37495
This is working great on iOS too

@Pranav-yadav
Copy link
Contributor

Sure. That seems great.
Let us know whenever this PR is ready for the review :)

function DrawerDefault() {
return (
<DrawerLayoutIos visible width={420}>
<View style={styles.modalContainer}></View>

Choose a reason for hiding this comment

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

  • ⚠️ packages/rn-tester/js/examples/DrawerLayoutIos/DrawerLayoutIosExample.js line 19 – Empty components are self-closing (react/self-closing-comp)

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 30, 2023
@javache
Copy link
Member

javache commented Oct 31, 2023

Why should this component go in React Native core? Would it be better served as a community package?

We have DrawerLayoutAndroid because it's a core Android SDK component, but it would be better if these weren't part of React Native's core.

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

See @javache comment. We're looking into making the core smaller, and we don't want to add other components like this one. Is there a strong argument to have this oen included?

@Gregoirevda Gregoirevda closed this by deleting the head repository Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Needs: Docs Website PR To encourage the authors to create a website PR for documentation. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants