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

[NavigationDrawer] Allow users to scroll to dismiss on lower resolution devices. #8503

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

codeman7
Copy link
Contributor

@codeman7 codeman7 commented Sep 25, 2019

This change allows currentlyFullscreen property on MDCBottomDrawerContainerViewController to become NO on lower resolution devices. Currently this property is always NO. This doesn't allow the code within - (void)scrollViewWillEndDragging:withVelocity:targetContentOffset: to be executed.

Testing

  1. Set preferredContentSize on the headerVC of a Navigation Drawer example to 80.
  2. Set preferredContentSize on the contentVC of the same Navigation Drawer example to a value less than 240.
  3. Open the example on a iPhoneSE
  4. Rotate to landscape.
  5. Try to swipe to dismiss the drawer.

Closes #8454

@material-automation
Copy link

material-automation commented Sep 25, 2019

bazel detected changes to the following targets:

//components/NavigationDrawer:ColorThemer
//components/NavigationDrawer:NavigationDrawer
//components/NavigationDrawer:SwiftExamples
//components/NavigationDrawer:snapshot_test_lib
//components/NavigationDrawer:snapshot_tests
//components/NavigationDrawer:snapshot_tests.swift_runtime_linkopts
//components/NavigationDrawer:snapshot_tests_test_binary
//components/NavigationDrawer:snapshot_tests_test_bundle
//components/NavigationDrawer:unit_test_sources
//components/NavigationDrawer:unit_tests
//components/NavigationDrawer:unit_tests_IPAD_PRO_12_9_IN_9_3
//components/NavigationDrawer:unit_tests_IPAD_PRO_12_9_IN_9_3.swift_runtime_linkopts
//components/NavigationDrawer:unit_tests_IPAD_PRO_12_9_IN_9_3_test_binary
//components/NavigationDrawer:unit_tests_IPAD_PRO_12_9_IN_9_3_test_bundle
//components/NavigationDrawer:unit_tests_IPHONE_7_PLUS_IN_10_3
//components/NavigationDrawer:unit_tests_IPHONE_7_PLUS_IN_10_3.swift_runtime_linkopts
//components/NavigationDrawer:unit_tests_IPHONE_7_PLUS_IN_10_3_test_binary
//components/NavigationDrawer:unit_tests_IPHONE_7_PLUS_IN_10_3_test_bundle
//components/NavigationDrawer:unit_tests_environment
//components/NavigationDrawer:unit_tests_environment.swift_runtime_linkopts
//components/NavigationDrawer:unit_tests_environment_test_binary
//components/NavigationDrawer:unit_tests_environment_test_bundle

@material-automation
Copy link

bazel detected changes to the following targets:

//components/NavigationDrawer:snapshot_tests
//components/NavigationDrawer:snapshot_tests_test_bundle
//components/NavigationDrawer:unit_tests
//components/NavigationDrawer:unit_tests_IPAD_PRO_12_9_IN_9_3
//components/NavigationDrawer:unit_tests_IPAD_PRO_12_9_IN_9_3_test_bundle
//components/NavigationDrawer:unit_tests_IPHONE_7_PLUS_IN_10_3
//components/NavigationDrawer:unit_tests_IPHONE_7_PLUS_IN_10_3_test_bundle
//components/NavigationDrawer:unit_tests_environment
//components/NavigationDrawer:unit_tests_environment_test_bundle

@yarneo
Copy link
Contributor

yarneo commented Sep 25, 2019

Could you please provide some documentation around the addition to explain why we need this change and what it means

@codeman7
Copy link
Contributor Author

@yarneo I've added documentation please let me know if further clarification is needed.

@yarneo
Copy link
Contributor

yarneo commented Sep 26, 2019

Just want to clarify:
prior to this change: currentlyFullscreen was always NO for lower resolution devices at all times? Landscape and Portrait, and when being deep into the content scroll view?

Do you believe maybe that your added change is possibly the only needed conditional and
self.contentReachesFullscreen && headerTransitionToTop >= 1 can be removed?

@@ -641,7 +641,8 @@ - (void)updateViewWithContentOffset:(CGPoint)contentOffset {
transitionRatio:transitionPercentage];

[self updateDrawerState:transitionPercentage];
self.currentlyFullscreen = self.contentReachesFullscreen && headerTransitionToTop >= 1;
self.currentlyFullscreen =
self.contentReachesFullscreen && contentOffset.y > 0;

Choose a reason for hiding this comment

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

clang-format suggested the following change:

-  self.currentlyFullscreen =
-      self.contentReachesFullscreen && contentOffset.y > 0;
+  self.currentlyFullscreen = self.contentReachesFullscreen && contentOffset.y > 0;

@codeman7
Copy link
Contributor Author

@yarneo headerTransitionToTop >= 1 can be removed but self.contentReachesFullScreen cannot.

Copy link
Contributor

@yarneo yarneo left a comment

Choose a reason for hiding this comment

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

Thanks. please make sure we haven't regressed with high res devices with this fix. good work

@codeman7
Copy link
Contributor Author

@yarneo I tested on high res devices and it looks good to me.

@codeman7 codeman7 merged commit a80e41c into material-components:develop Sep 26, 2019
codeman7 added a commit that referenced this pull request Sep 26, 2019
In #8503 this line was removed but after further testing when the contentViewController expands it's preferredContentSize this causes a regression in the header behavior. With this line added back into the check this behavior is fixed.
ggdiez pushed a commit to solidgear/material-components-ios that referenced this pull request Sep 27, 2019
…on devices. (material-components#8503)

This change allows `currentlyFullscreen` property on [`MDCBottomDrawerContainerViewController`](https://github.com/material-components/material-components-ios/blob/develop/components/NavigationDrawer/src/private/MDCBottomDrawerContainerViewController.m) to become `NO` on lower resolution devices. Currently this property is always `NO`. This doesn't allow the code within `- (void)scrollViewWillEndDragging:withVelocity:targetContentOffset:` to be executed.

## Testing
1. Set `preferredContentSize` on the headerVC of a Navigation Drawer example to 80.
2. Set `preferredContentSize` on the contentVC of the same Navigation Drawer example to a value less than 240. 
3. Open the example on a iPhoneSE
4. Rotate to landscape.
5. Try to swipe to dismiss the drawer.

Closes #8454
ggdiez pushed a commit to solidgear/material-components-ios that referenced this pull request Sep 27, 2019
…omponents#8514)

In material-components#8503 this line was removed but after further testing when the contentViewController expands it's preferredContentSize this causes a regression in the header behavior. With this line added back into the check this behavior is fixed.
@codeman7 codeman7 deleted the nav-drawer-scroll branch October 4, 2019 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants