[Drawer] Proper placement for anchor other than left #6516
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Assume a default of 'left' - Check right to left, swap 'right' and 'left' - Set initial placement and height based on the anchor property - Consideration of right-to-left when anchor is set to right - For top and bottom, height is set to auto, the max height is left for the consumer of this component since Material Design does not prevent a full-height bottom sheet. No word on top sheets. - Added top, bottom, right drawers to component demo.
|
Associated this PR with Issue #6517 |
muibot
added
PR: needs revision
- left anchor should switch to right - right anchor should switch to left
mbrookes
added
component: drawer
oliviertassinari
requested changes
Apr 7, 2017
|
|
||
| render() { | ||
| const classes = this.context.styleManager.render(styleSheet); | ||
| return ( | ||
| <div> | ||
| <Button onClick={this.handleOpen}>Open Drawer</Button> | ||
| <Button onClick={this.handleTopOpen}>Open Top Drawer</Button> |
There was a problem hiding this comment.
I would have the left drawer first in the list as people will most likely try that one first.
|
|
||
| render() { | ||
| const classes = this.context.styleManager.render(styleSheet); | ||
| return ( | ||
| <div> | ||
| <Button onClick={this.handleOpen}>Open Drawer</Button> | ||
| <Button onClick={this.handleTopOpen}>Open Top Drawer</Button> | ||
| <Button onClick={this.handleLeftOpen}>Open Left Drawer</Button> |
There was a problem hiding this comment.
I think that we can remove Drawer from the button wording, that's obvious and making it harder to read.
| <ListItemText primary="Spam" /> | ||
| </ListItem> | ||
| </List> | ||
| <div className={classes.remainder} /> |
There was a problem hiding this comment.
I'm not sure we need a remainder.
| onRequestClose={this.handleTopClose} | ||
| onClick={this.handleTopClose} | ||
| > | ||
| <List className={classes.listFull} padding={false}> |
There was a problem hiding this comment.
The property changed, it's disablePadding
| </ListItem> | ||
| </List> | ||
| <Divider /> | ||
| <List className={classes.listFull} padding={false}> |
There was a problem hiding this comment.
I think that we should keep the children of the list DRY. Please use an intermediary constant.
src/Drawer/Drawer.js
Outdated
| @@ -126,7 +151,18 @@ export default class Drawer extends Component { | |||
| const { theme: { dir }, render } = this.context.styleManager; | |||
| const classes = render(styleSheet); | |||
| const rtl = dir === 'rtl'; | |||
| const anchor = anchorProp || (rtl ? 'right' : 'left'); | |||
| const anchorClasses = { | |||
There was a problem hiding this comment.
We could have been using smart code, but I guess a mapping is simpler here 👍
oliviertassinari
added
next
PR: needs revision
- reordered buttons in drawer demo - removed drawer from button label in demo - eliminated remainder in demo - made use of constant for reused list - eliminated mapping, renamed classes to match acceptable values for anchor prop, apply appropriate class by using value of anchor as a key for the classes object
|
Thanks for reviewing, @oliviertassinari - I've pushed the requested changes. |
| @@ -140,7 +169,7 @@ export default class Drawer extends Component { | |||
| <Paper | |||
| elevation={docked ? 0 : elevation} | |||
| square | |||
| className={classNames(classes.paper, paperClassName)} | |||
| className={classNames(classes.paper, classes[anchor], paperClassName)} | |||
There was a problem hiding this comment.
Now, I would call that a smart code, I'm not sure it's better as harder to debug, but if that what you prefer, I'm in 👍 .
- should have full width list for top and bottom
oliviertassinari
approved these changes
Apr 7, 2017
oliviertassinari
added
PR: Review Accepted
and removed
PR: Needs Review
PR: needs revision
|
@kgregory Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The anchor property is not fully implemented. It appropriately sets the direction that the drawer should slide, but the drawer is always placed at the left of the viewport. This is because the initial position and height is not changed when anchor is right, top, or bottom.
Resolution: