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

[BottomSheet] Add BottomSheet component #4336

Closed
wants to merge 1 commit into from

Conversation

antialiasis
Copy link

@antialiasis antialiasis commented May 25, 2016

Hey, first time contributing. I needed a BottomSheet component for a project, and it seemed appropriate to submit it back to Material-UI.

It follows the spec reasonably closely, except for the part about the sheet not initially expanding beyond the 16:9 keyline on mobile - as far as I'm aware material-ui doesn't really do specialized mobile behaviour as it is and has no facilities to support that, although since I'm not that familiar with the entire codebase I may be mistaken.

Following that, I was unsure how to deal with the possibility in the spec of fully expanding a bottom sheet - the way it's described there, the sheet starts out at the 16:9 keyline but can be expanded by swiping it up, but if it's not starting at the 16:9 keyline, that doesn't make sense (either the content is small enough to fit, in which case the sheet'll fit the content and shouldn't be expanded, or the content is bigger than the screen, in which case the sheet's max-height will take effect in accordance with another part of the spec, making it effectively fully expanded from the start).

If you have any thoughts on doing this better, I'm all for improving the pull request.

Includes docs demo, but no tests - this is heavily based on the Drawer component, which has no tests, and I wasn't sure exactly what you would want tested here, if anything - I can't tell if the slash in that checklist item means "and" or "or".

Closes #2126 (although see comments above on fully expanded sheets).

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@fannarsh
Copy link

@oliviertassinari and @mbrookes any comments on this PR?

@mbrookes
Copy link
Member

mbrookes commented Jun 2, 2016

@antialiasis Thanks for the contribution. Did you consider making this a feature of Drawer?

@antialiasis
Copy link
Author

Hi, thanks for responding.

While this component is based on the Drawer and has many similar features, there are also significant differences. For example, while a drawer has a set width, according to the spec a bottom sheet should adapt to the height of its content; this necessitates a somewhat different approach to how we keep it shifted out of frame while closed. Bottom sheets should be closeable by sliding down, like drawers are closeable by sliding towards the edge, but unlike with drawers, we can't generally decide whether the user is trying to scroll or slide based on the direction they move in but rather must decide based on where the touch originates. Some drawer defaults just don't make sense for bottom sheets: as far as I can tell from the spec, bottom sheets are never supposed to open when sliding up from the bottom, but to accomplish this with a drawer the user needs to explicitly set disableSlideToOpen, and following from that, it doesn't really make sense for bottom sheets to be uncontrolled, either. It'd be possible to combine the two sensibly to make a generic drawer component that can open from any side of the screen, but it'd call for a lot of probably backwards-incompatible changes to the Drawer component and involve quite a bit of specialized behaviour based on whether it's horizontal or vertical. Perhaps it'd make sense to do that anyway, but all in all a new component seemed like the more conservative approach, compared to cutting up an existing one, introducing backwards-incompatibilities and possibly breaking existing behaviour.

I would also say that drawers and bottom sheets, while similar in the sense that they slide in from an edge of the screen and can be closed by sliding back, are conceptually pretty different and made for different use cases. Drawers are navigation menus and a page generally has exactly one of them (possibly two); they are top-level objects. Bottom sheets are either freeform extra content/information/controls supplementing the main view or essentially a mobile alternative to drop-down menus or dialogs; they're fundamentally context-sensitive. This is why they call for different defaults.

It's true the bottom sheet component repeats a lot of Drawer's functionality, though. Perhaps it'd be ideal to have a generic component implementing the slide-in/out functionality from any direction and have both Drawer and BottomSheet make use of that.

@mbrookes
Copy link
Member

mbrookes commented Jun 2, 2016

Perhaps it'd be ideal to have a generic component implementing the slide-in/out functionality from any direction and have both Drawer and BottomSheet make use of that.

That sounds like a sensible approach, otherwise we'll find ourselves maintaining multiple variations of the same code in different components. Bottom Nav could also potentially take advantage in the future.

@nathanmarks
Copy link
Member

@antialiasis 100% about the functionality repetition.

Yes, when it comes down to specifics they are different. However, they share a significant amount of underlying concepts/functionality and I believe we'd be better off finding a design that promotes more code sharing in this situation.

I'd love to hear/see any ideas you have.

@antialiasis
Copy link
Author

I'll do the proposed refactor of the sliding functionality into a separate single component that can be used by both Drawer and BottomSheet and possible future components, then, and you can see what you think.

@antialiasis
Copy link
Author

Hello again. I've refactored the shared drawer/bottom sheet functionality into a shared internal component, SlidingSheet. I believe everything should work as before in both Drawer and BottomSheet.

@oliviertassinari
Copy link
Member

@antialiasis Interesting PR, I'm gonna have a look at it. Thanks for working on this!

@@ -0,0 +1,31 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

import React, {Component} from 'react';

@oliviertassinari
Copy link
Member

@antialiasis I will continue reviewing this PR tomorrow. Great start 👍 .
It would be good to rebase on top of master, I had to run npm install.


close(reason) {
if (this.props.onRequestClose) this.props.onRequestClose(reason);
return this;
Copy link
Member

Choose a reason for hiding this comment

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

Why returning this? close is a private API, we don't need to chain the calls.

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Jun 12, 2016
@antialiasis
Copy link
Author

Hello and thanks for the review! Apologies for not getting back to you earlier; I've been on vacation. I'll take a stab at improving the PR according to your comments soon.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 29, 2016

@antialiasis Great 🌴 , thanks!

@mbrookes mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jul 7, 2016
@antialiasis
Copy link
Author

Hello again! I've made the mentioned improvements to the PR. Since most of the issues mentioned were actually inherited from Drawer (should have made sure to familiarize myself with current practices!), and the PR makes changes to Drawer anyway, I fixed them there as well; also rebased the whole thing off master, as suggested.

@oliviertassinari
Copy link
Member

@antialiasis That's a great contribution! Thanks a lot. I'm have noticed more stuff that we could improve on this PR. I'm gonna carry it to master. Expect a new PR soon starting from your commit.

@muibot muibot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: Needs Review labels Jul 19, 2016
@skirunman
Copy link
Contributor

I saw updated roadmap and plan was to minimize new features in 0.15.3. However, this one plus support for bottom navigation are key for our mobile app #3712

Would be awesome to have these features in the next release so we at least have something to work with before the major refactor release 0.16 is available in a few months. Thanks for the consideration.

@oliviertassinari
Copy link
Member

I'm closing this PR as I have based #4732 on top of this one. It would be better to keep one place to get feedbacks.
Thanks for this contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants