Support drawer menus opening as overlay instead of pushing content in ios #7986
Conversation
… ios (#7984) * add example in playground and some animations changes with Yogi * fix drag animations (open+close) in left drawer and add overlay on center * support all features in right drawer * supoprt both old and new opening mode using openAboveScreen param * connect the new parameter to rnn platform * update param from boolean to enum * clean * install * update lock * fix unit
d4vidi
force-pushed
the
support-drawer-menu-open-mode
branch
from
0a1937a to
f1d3449
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces overlay mode for side drawer menus on iOS, allowing the drawer to open above the content rather than pushing it.
- Added new test IDs for left and right menus.
- Added new functions and UI buttons in SetRootScreen to configure left and right menus with overlay behavior.
- Updated the Options interface to support the new openMode property.
Reviewed Changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| playground/src/testIDs.ts | Added new test IDs for the left and right side menus. |
| playground/src/screens/SetRootScreen.tsx | Added buttons and functions to configure side menus in overlay mode for iOS. |
| lib/src/interfaces/Options.ts | Extended the Options interface with an openMode property for side menus. |
Files not reviewed (5)
- lib/ios/RNNSideMenu/MMDrawerController/MMDrawerController.h: Language not supported
- lib/ios/RNNSideMenuPresenter.m: Language not supported
- lib/ios/RNNSideMenuSideOptions.h: Language not supported
- lib/ios/RNNSideMenuSideOptions.m: Language not supported
- playground/ios/NavigationTests/RNNSideMenuPresenterTest.m: Language not supported
Comments suppressed due to low confidence (2)
playground/src/screens/SetRootScreen.tsx:336
- [nitpick] The component id 'sideMenu' is used for the left menu configuration; consider renaming it to 'leftSideMenu' for clarity and to avoid confusion with the right menu configuration.
id: 'sideMenu',
playground/src/screens/SetRootScreen.tsx:376
- [nitpick] The component id 'sideMenu' is used for the right menu configuration; consider renaming it to 'rightSideMenu' for clearer distinction between the left and right menus.
id: 'sideMenu',
| <Button | ||
| label="Set Root with left menu" | ||
| testID={SET_ROOT_WITH_LEFT_MENU} | ||
| onPress={this.setRootWithLeftMenu} | ||
| /> | ||
| <Button | ||
| label="Set Root with right menu" | ||
| testID={SET_ROOT_WITH_RIGHT_MENU} | ||
| onPress={this.setRootWithRightMenu} | ||
| /> |
There was a problem hiding this comment.
I think better place for this would be the side-menu modal (Layouts > SideMenu btn) --
...
- Did you resort to set-root on purpose, in order to simplify?
- If so, why split the left-menu and right-menu into 2 different sub-screens, rather than having 1 screen with both? It's work quirky this way, with the "right menu" button breaking the app when set with the left-menu only
There was a problem hiding this comment.
- it's in root and not modal cause it shows our actual use case, we set the drawer in the root hierarchy - modal is different case
- np, changing to one button with both menus :)
There was a problem hiding this comment.
Definitely need to add this to at least one or more e2e test case, with screenshot comparison.
lib/src/interfaces/Options.ts
Outdated
| * Configure the opening mode of the side menu | ||
| * #### (iOS specific) | ||
| * @default 'pushContent' | ||
| * @default 'pushContent' | ||
| */ | ||
| openMode?: 'pushContent' | 'aboveContent'; |
There was a problem hiding this comment.
openMode - to me, sounds like a configuration of which animation to use when opening the drawer.
Suggestion: Rename openMode → layoutType
There was a problem hiding this comment.
BTW is shouldStretchDrawer applicable with aboveContent? If not, need to specify that too in the description
There was a problem hiding this comment.
what shouldStretchDrawer is for? it should be default true but I can't see any indication to something like stretching content..
There was a problem hiding this comment.
regarding the renaming layoutType sounds to general to me, I will prefer to keep the openMode naming 🙏
lib/ios/RNNSideMenuSideOptions.m
Outdated
| MMDrawerOpenMode MMDrawerOpenModeFromString(NSString *openModeString) { | ||
| if (!openModeString) { | ||
| return MMDrawerOpenModePushContent; // Default | ||
| } | ||
|
|
||
| if ([openModeString isEqualToString:@"aboveContent"]) { | ||
| return MMDrawerOpenModeAboveContent; | ||
| } else { | ||
| // Default or explicit "pushContent" | ||
| return MMDrawerOpenModePushContent; | ||
| } | ||
| } |
There was a problem hiding this comment.
MMDrawerOpenMode MMDrawerOpenModeFromString(NSString *openModeString) {
if ([openModeString isEqualToString:@"aboveContent"]) {
return MMDrawerOpenModeAboveContent;
}
return MMDrawerOpenModePushContent;
}(isEqualToString should be checking implicitly for non-nullability)
| NSTimeInterval duration = MAX(distance / ABS(velocity), MMDrawerMinimumAnimationDuration); | ||
|
|
||
| // Animate opening | ||
| [UIView animateWithDuration:(animated ? duration : 0.0) |
There was a problem hiding this comment.
This call needs to be extracted onto a function and reused in all modes (overlay/push)
There was a problem hiding this comment.
in each call the settings are different we can't share this code, it's not really exactly the same 🤔 maybe I'm missing something?
| self.rightDrawerOpenMode; | ||
|
|
||
| if (openMode == MMDrawerOpenModeAboveContent) { | ||
| // OVERLAY MODE |
There was a problem hiding this comment.
2 dedicated sub-functions: One for each mode (overlay, push-content)
| NSTimeInterval duration = MAX(distance / ABS(velocity), MMDrawerMinimumAnimationDuration); | ||
|
|
||
| // Animate closure | ||
| [UIView animateWithDuration:(animated ? duration : 0.0) |
There was a problem hiding this comment.
This code needs to be extracted onto a function and shared across all mode
There was a problem hiding this comment.
in each call the settings are different we can't share this code 🤔 maybe I'm missing something?
| self.rightDrawerOpenMode; | ||
|
|
||
| if (openMode == MMDrawerOpenModeAboveContent) { | ||
| // OVERLAY MODE |
There was a problem hiding this comment.
2 separate functions for closing the overlay in each mode (maybe even just 1 closing logic func can be enough?...)
| self.leftDrawerOpenMode : | ||
| self.rightDrawerOpenMode; | ||
|
|
||
| if (openMode == MMDrawerOpenModeAboveContent) { |
There was a problem hiding this comment.
Really nice, but why is this custom handling needed just for the overlay mode (but not push mode)?
There was a problem hiding this comment.
Anyways it would be impossible to merge this code as-is. It needs to be extracted onto a separate module, to handle all cases - UIGestureRecognizerStateBegan, UIGestureRecognizerStateChanged and UIGestureRecognizerStateEnded. Preferably, it should be delegated to the original mode's code which I assume already has this (?)....
There was a problem hiding this comment.
@d4vidi I don't understand the second comment, can you please clarify? 🙏
regarding the first question the whole calculation is different between the modes cause in the original mode we calculate the center view position and in the new mode we needs the drawer itself
the actual element who moves is different
thats why there are many differences between the two regarding the animation calculations
liatnetach
force-pushed
the
support-drawer-menu-open-mode
branch
from
f1d3449 to
4803f82
Compare
|
#rebuild |
This PR adds the ability to configure how side drawer menus open, introducing an overlay mode in addition to the traditional push-content behavior.