Conversation
…y handling. Needs tests, variables on styles
mdespuits
left a comment
mdespuits
left a comment
There was a problem hiding this comment.
Couple of small things. Liking the use of hooks!
…ve redundant variable import
|
I committed a change to add a SideTray to the example page. Would you verify it doesn't break your subsequent changes? Also, I tested in IE11 and Edge - looking good!! |
| @@ -1,5 +1,5 @@ | |||
| .sideTray { | |||
| width: var(--rvr-side-tray-width); | |||
There was a problem hiding this comment.
Why are we losing the variable here?
There was a problem hiding this comment.
Hey @mdespuits,
I suggested this. I figured that:
- The sidetray width isn't intended for re-use among components.
- There's nothing magical about 400 px in our sizing system.
- It's not part of the theme.
I could see us standardizing on some sizes for modals / popovers / etc. in the future, but for now I thought this was cleaner than having a block of sidebar-specific properties in our global variables.
mdespuits
left a comment
mdespuits
left a comment
There was a problem hiding this comment.
A couple of thoughts and a question or two
|
Good catch on the imports in stories.i think that's because storybook used a different webpack config and therefore different css loader module options, Importing shared css directly for now in stories is great. |
| children: PropTypes.node.isRequired, | ||
| }; | ||
|
|
||
| const Header = props => <div {...props} className={style.SideTray__Header} />; |
There was a problem hiding this comment.
Soooo close - but we should still support consumer-driven additional classes, I think.
So {...props} className={`${style.SideTray__Header} ${props.className}`}
|
I'm drunk with power and seeing if I can dismiss @mdespuits review, but only because I already verified that @chelshaw made the changes requested. |
pixelbandito
dismissed
mdespuits’s stale review
@chelsea made the changes requested.
4 participants
SUMMARY
Add a sidetray component that calls closeCallback when:
esckeyHas props for header and footer nodes, and content between them automatically flexGrows