[BottomSheet] Add property so clients can set a custom height for bottom sheet #5139

codeman7 · 2018-09-17T13:12:20Z

Previously clients could only set a custom height for bottom sheet by setting the preferredContentSize this adds a property so clients can set a specific height and not touch preferredContentSize.

Related to #4945

material-automation · 2018-09-17T13:14:06Z

...nents/BottomSheet/tests/unit/MDCBottomSheetPresentationControllerPreferredSheetHeightTests.m

+  self.presentationController.preferredSheetHeight = preferredSheetHeight;
+
+  // When
+  [self.presentationController updatePreferredSheetHeight:self.presentationController.presentedViewController.preferredContentSize.height];


clang-format suggested the following change:

- [self.presentationController updatePreferredSheetHeight:self.presentationController.presentedViewController.preferredContentSize.height]; + [self.presentationController + updatePreferredSheetHeight:self.presentationController.presentedViewController + .preferredContentSize.height];

material-automation · 2018-09-17T13:14:07Z

...nents/BottomSheet/tests/unit/MDCBottomSheetPresentationControllerPreferredSheetHeightTests.m

+  self.presentationController.preferredSheetHeight = preferredSheetHeight;
+
+  // When
+  [self.presentationController updatePreferredSheetHeight:self.presentationController.presentedViewController.preferredContentSize.height];


clang-format suggested the following change:

- [self.presentationController updatePreferredSheetHeight:self.presentationController.presentedViewController.preferredContentSize.height]; + [self.presentationController + updatePreferredSheetHeight:self.presentationController.presentedViewController + .preferredContentSize.height];

material-automation · 2018-09-17T13:14:42Z

API diff detected the following changes

BottomSheet

MDCBottomSheetTransitionController

new property: preferredSheetHeight in MDCBottomSheetTransitionController

MDCBottomSheetPresentationController

new property: preferredSheetHeight in MDCBottomSheetPresentationController

jverkoey

We may need a test that checks what happens when preferredSheetHeight is set to a positive value and then back to zero.

jverkoey · 2018-09-17T15:12:39Z

components/BottomSheet/src/MDCBottomSheetPresentationController.h

+
+ @note This will override setting the preferredContentSize if a positive value is passed.
+ @note If a non-positive value is passed then the sheet will open up to either half the screen
+ height or the size of the contentViewController whatever value is smaller.


tip: I find documentation like this can be a bit easier to parse if written as a logical statement, like so:

If this, then this. Otherwise, this.

So in this case:

If a positive value is passed, overrides preferredContentSize. Otherwise, the sheet will open up to...

jverkoey · 2018-09-17T15:15:58Z

components/BottomSheet/src/MDCBottomSheetPresentationController.h

+/**
+ This is used to set a custom height on the sheet view.
+
+ @note This will override setting the preferredContentSize if a positive value is passed.


What does override mean? Please add the answer as documentation.

I think I made this more clear.

jverkoey · 2018-09-17T15:19:23Z

components/BottomSheet/src/MDCBottomSheetPresentationController.m

@@ -179,9 +179,11 @@ - (void)viewWillTransitionToSize:(CGSize)size

 @param preferredSheetHeight If positive, the new value for @sheetView.preferredSheetHeight.
 */
- (void)setPreferredSheetHeight:(CGFloat)preferredSheetHeight {
+- (void)updatePreferredSheetHeight:(CGFloat)preferredSheetHeight {


I think this method doesn't need an argument because you're able to pull the information from either self.presentedViewController.preferredContentSize.height or self.preferredSheetHeight depending on the value of self.preferredSheetHeight. Removing this argument may simplify both this method and the call sites as a result.

The method name in this case could likely be renamed to updatePreferredSheetHeight or something similar to indicate that the value has already changed and we now want to update the sheetView's preferredSheetHeight property accordingly.

I believe #5125 updated this method to make it more testable, I can revert that change if you'd like.

Ah! I see.

I think we don't need to revert that change - you're increasing the surface area of the APIs here to include both a setter and a "did change" method that commits the results to the sheetView.

Prior to this PR, the tests relied on the method previously named setPreferredSheetHeight. The tests were using this method to propagate changes to the sheetView.

After this PR, setPreferredSheetHeight has a different meaning and you're introducing a new method, updatePreferredSheetHeight that models the old meaning of setPreferredSheetHeight.

You can still test as before, but your existing tests will now call the updatePreferredSheetHeight API to propagate changes to the sheetView instead of setPreferredSheetHeight, except in cases where you're testing the new property's behavior specifically.

Tying this back to my original suggestion, if you remove updatePreferredSheetHeight's argument then your tests will still be able to call this method, but prior to doing so they must set the relevant properties that they are testing (either preferredSheetHeight or preferredContentSize.height).

Spoke over gvc:

Partial revert of [BottomSheet] Use setter for sheet view height. #5125 so that we still have an updatePreferredSheetHeight method that doesn't accept arguments.

updatePreferredSheetHeight will be exposed to tests so that we can "flush" the calculated sheet height to sheetView once we've configured either preferredSheetHeight or preferredContentSize.height.

Also discussed the possibility of making updatePreferredSheetHeight more testable by making it stateless, but that doing so can be a follow-up exploration:

+ (CGFloat)preferredSheetHeightForViewController:(UIViewController *)viewController withFrame:(CGRect)frame preferredSheetHeight:(CGFloat)preferredSheetHeight { CGFloat calculatedPreferredSheetHeight = (preferredSheetHeight > 0.f) ? preferredSheetHeight : viewController.preferredContentSize.height; if (MDCCGFloatEqual(calculatedPreferredSheetHeight, 0)) { calculatedPreferredSheetHeight = MDCRound(CGRectGetHeight(frame) / 2); } return calculatedPreferredSheetHeight; }

jverkoey · 2018-09-17T15:20:57Z

components/BottomSheet/src/MDCBottomSheetPresentationController.m

+  _preferredSheetHeight = preferredSheetHeight;
+  if (preferredSheetHeight > 0.f) {
+    [self updatePreferredSheetHeight:preferredSheetHeight];
+  }


You can remove the if statement here if updatePreferredSheetHeight is changed to a updatePreferredSheetHeight-style method.

Pending your feedback on reverting #5125.

jverkoey

I'm afraid I'm still not certain I understand when someone would set preferredSheetHeight vs preferredContentSize.height. As best I can tell from the code these two properties have equivalent results.

Can you share some additional context that might help me understand the need for this new property?

jverkoey · 2018-09-17T15:27:38Z

components/BottomSheet/src/MDCBottomSheetPresentationController.h

+ @note This will override setting the preferredContentSize if a positive value is passed.
+ @note If a non-positive value is passed then the sheet will open up to either half the screen
+ height or the size of the contentViewController whatever value is smaller.
+ */


Looking at the implementation of MDCSheetContainerView.m, it looks like truncatedPreferredSheetHeight always considers the contentViewController's height when determining the maximum height. This implies that preferredSheetHeight will also be capped to the content's height, which the documentation doesn't yet indicate. We may want to add a test and documentation indicating this.

E.g. pseudo-code:

preferredSheetHeight = 1000 contentViewController.scrollView.contentSize.height = 500 assertEqual(sheet.height, 500) // Docs presently imply 1000

codeman7 · 2018-09-17T15:48:25Z

Setting the preferredContentSize also effects the frame of the UIViewController.view so when you rotate or change view.frame it isn't correct, it will be the value of preferredContentSize. This makes it so that the view.frame will always be correct.

jverkoey · 2018-09-17T15:50:00Z

Ah! Interesting. Who sets view.frame in reaction to preferredContentSize? Is that us?

codeman7 · 2018-09-17T15:54:57Z

In order to close #4945 I would either have to set preferredContentSize or this new property.

romoore · 2018-09-18T02:23:22Z

...nents/BottomSheet/tests/unit/MDCBottomSheetPresentationControllerPreferredSheetHeightTests.m

@@ -107,10 +107,12 @@ - (void)testSetPreferredSheetHeightZeroWhenSheetViewHasUnstandardizedFrame {
 - (void)testSetPreferredSheetHeightPositiveValue {


Super-nit, please fix up the test method names. They are no longer testing setPreferredSheetHeight.

material-automation · 2018-09-18T14:34:55Z

...nents/BottomSheet/tests/unit/MDCBottomSheetPresentationControllerPreferredSheetHeightTests.m

+  self.presentationController.sheetView = sheetView;
+  self.presentationController.preferredSheetHeight = 5000;
+
+


clang-format suggested the following change:

-

New commit

jverkoey

Awesome! I love how clear this is now.

Initial commit

ffc1ed2

material-automation bot added the [BottomSheet] label Sep 17, 2018

googlebot added the cla: yes label Sep 17, 2018

codeman7 requested a review from a team September 17, 2018 13:12

material-automation reviewed Sep 17, 2018

View reviewed changes

Clang

f6b12ba

material-automation bot added the [BottomSheet] label Sep 17, 2018

jverkoey reviewed Sep 17, 2018

View reviewed changes

Address feedback

f36cb41

jverkoey previously requested changes Sep 17, 2018

View reviewed changes

jverkoey reviewed Sep 17, 2018

View reviewed changes

Address feedback

2cfeb20

romoore reviewed Sep 18, 2018

View reviewed changes

codeman7 added 3 commits September 18, 2018 10:09

Large test

e38b1b2

Refactor test

624d7c2

Update test

7d5ffc4

material-automation reviewed Sep 18, 2018

View reviewed changes

codeman7 added 2 commits September 18, 2018 10:35

Update naming

d00df45

Clang

7aed028

codeman7 requested a review from jverkoey September 18, 2018 14:38

jverkoey approved these changes Sep 18, 2018

View reviewed changes

codeman7 merged commit 553fb01 into material-components:develop Sep 18, 2018

codeman7 deleted the bottom-sheet-height branch October 17, 2018 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BottomSheet] Add property so clients can set a custom height for bottom sheet #5139

[BottomSheet] Add property so clients can set a custom height for bottom sheet #5139

codeman7 commented Sep 17, 2018 •

edited

material-automation Sep 17, 2018

material-automation Sep 17, 2018

material-automation commented Sep 17, 2018

jverkoey left a comment

jverkoey Sep 17, 2018

codeman7 Sep 17, 2018

jverkoey Sep 17, 2018

codeman7 Sep 17, 2018

jverkoey Sep 17, 2018

jverkoey Sep 17, 2018

codeman7 Sep 17, 2018

jverkoey Sep 17, 2018

jverkoey Sep 17, 2018 •

edited

jverkoey Sep 17, 2018

jverkoey Sep 17, 2018 •

edited

jverkoey Sep 17, 2018 •

edited

codeman7 Sep 17, 2018

jverkoey left a comment

jverkoey Sep 17, 2018

codeman7 commented Sep 17, 2018

jverkoey commented Sep 17, 2018

codeman7 commented Sep 17, 2018

romoore Sep 18, 2018

material-automation Sep 18, 2018

jverkoey left a comment

		@@ -107,10 +107,12 @@ - (void)testSetPreferredSheetHeightZeroWhenSheetViewHasUnstandardizedFrame {
		- (void)testSetPreferredSheetHeightPositiveValue {

		self.presentationController.sheetView = sheetView;
		self.presentationController.preferredSheetHeight = 5000;

[BottomSheet] Add property so clients can set a custom height for bottom sheet #5139

[BottomSheet] Add property so clients can set a custom height for bottom sheet #5139

Conversation

codeman7 commented Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

material-automation commented Sep 17, 2018

API diff detected the following changes

BottomSheet

MDCBottomSheetTransitionController

MDCBottomSheetPresentationController

jverkoey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jverkoey Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jverkoey Sep 17, 2018 • edited

Choose a reason for hiding this comment

jverkoey Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jverkoey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeman7 commented Sep 17, 2018

jverkoey commented Sep 17, 2018

codeman7 commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jverkoey left a comment

Choose a reason for hiding this comment

codeman7 commented Sep 17, 2018 •

edited

jverkoey Sep 17, 2018 •

edited

jverkoey Sep 17, 2018 •

edited

jverkoey Sep 17, 2018 •

edited