-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat(bottom-sheet): add isLockedOpen option; general cleanup #9179
Conversation
@@ -189,16 +197,15 @@ function MdBottomSheetProvider($$interimElementProvider) { | |||
$animate.enter(backdrop, options.parent, null); | |||
} | |||
|
|||
var bottomSheet = new BottomSheet(element, options.parent); | |||
options.bottomSheet = bottomSheet; | |||
options.cleanupGestures = options.isLockedOpen ? angular.noop : new BottomSheetGestures(element, options.parent); |
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.
This is pretty unclear.
if (!options.isLockedOpen) {
options.cleanupGestures = registerGestures(element, options.parent);
}
options.cleanupGestures && options.cleanupGestures()
function registerGestures(element, parent) {
...
}
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.
I agree about the naming, but the main reason I decided to default to a noop was to make the API consistent, no matter the options.
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.
It doesn't make any sense to have a noop function here. Check for example the dialog interim element / component.
Regarding API, the options are not any API for the developers. At time of show
those are just internal and used to store things between the different states.
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.
Ah right, I confused it with the hide/cancel/show on the service. I'll switch it up.
6485ec0
to
3c8ea26
Compare
var bottomSheet = new BottomSheet(element, options.parent); | ||
options.bottomSheet = bottomSheet; | ||
if (!options.isLockedOpen) { | ||
options.cleanupGestures = registerGestures(element, options.parent); |
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.
Actually after looking again at the PR, can't you move that declaration up to the isLockedOpen
check and use a else
?
if (options.isLockedOpen) {
options.clickOutsideToClose = false;
options.escapeToClose = false;
} else {
options.cleanupGestures = registerGestures(element, options.parent);
}
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.
I will, another good point 👍
LGTM aside from the one comment. |
3c8ea26
to
3eb8762
Compare
* Adds the `isLockedOpen` option to the bottom sheet service, allowing users to disable all non-programmatic ways of closing a bottom sheet. * Cleans up some redundant logic in the mdBottomSheet service. * Cleans up the bottom sheet unit test setup. Fixes angular#9084.
3eb8762
to
9000bb7
Compare
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.
LGTM
…#9179) * Adds the `isLockedOpen` option to the bottom sheet service, allowing users to disable all non-programmatic ways of closing a bottom sheet. * Cleans up some redundant logic in the mdBottomSheet service. * Cleans up the bottom sheet unit test setup. Fixes angular#9084.
* Adds the `isLockedOpen` option to the bottom sheet service, allowing users to disable all non-programmatic ways of closing a bottom sheet. * Cleans up some redundant logic in the mdBottomSheet service. * Cleans up the bottom sheet unit test setup. Fixes #9084.
isLockedOpen
option to the bottom sheet service, allowing users to disable all non-programmatic ways of closing a bottom sheet.Fixes #9084.