add Collapse component #79#201
add Collapse component #79#201eddywashere merged 24 commits intoreactstrap:masterfrom LiJinyao:collapse
Conversation
| this.element.style.height = `${this.getHeight()}px`; | ||
| }); | ||
| this.transitionTag = setTimeout(() => { | ||
| this.setState({ collapse: SHOWN }); |
There was a problem hiding this comment.
I believe the bootstrap js removes the height inline style after it has been shown. This would likely prevent any issues when added content and then collapsing the content again.
There was a problem hiding this comment.
Thanks for your review! The height inline style will be removed after setState called. Because there is no style declaration in the render function, so the inline height will be removed after rerender.
There was a problem hiding this comment.
Oh Sorry! I found the problem. I'll fix it.
| import Collapse from '../Collapse'; | ||
|
|
||
| describe('Collapse', () => { | ||
| it('should render children', () => { |
There was a problem hiding this comment.
Test coverage went down. Could you setup jasmine.clock here since you're using setTimeout.
beforeEach(() => {
jasmine.clock().install();
});
afterEach(() => {
jasmine.clock().uninstall();
});After prop changes, I believe you'll need to then use, jasmine.clock().tick(400); to move the virtual clock forward and test for the changes.
| '/components/tabs/', | ||
| '/components/jumbotron/', | ||
| '/components/alerts/', | ||
| '/components/collapse', |
There was a problem hiding this comment.
can you add a trailing / to generate the correct route path. I noticed a console warning regarding the active class applied to the nav .
| expect(wrapper.instance().props.isOpen).toEqual(false); | ||
| }); | ||
|
|
||
| it('should render with calss "collapse"', () => { |
There was a problem hiding this comment.
couple of typos here, calss -> class
| // will open | ||
| this.setState({ collapse: SHOW }, () => { | ||
| // start transition | ||
| this.element.style.height = `${this.getHeight()}px`; |
There was a problem hiding this comment.
What are your thoughts on moving these dom attribute changes over to react via setState? Would it still work or would the timing be off? I know some components break outside the react way of doing things but I'd like to only do that when absolutely necessary. This change doesn't have to happen in this PR either.
// apply via setState
setState({height: `${this.getHeight()}px`})
// in render
<div
{...attributes}
style={...attributes.style, height: this.state.height}
className={classes}
ref={(c) => { this.element = c; }}
/>
There was a problem hiding this comment.
I thought the timing will be off at first, now I am trying to move these dom attribute changes to react. Let's see if it will work fine.
| collapseClass | ||
| ); | ||
| return ( | ||
| <div |
There was a problem hiding this comment.
Could you update this to use the Tag (tag) prop pattern that other components use?
| super(props); | ||
| this.state = { | ||
| collapse: props.isOpen ? SHOWN : HIDDEN, | ||
| height: 0, |
There was a problem hiding this comment.
Should height be set to null isOpen is true?
There was a problem hiding this comment.
Ops, Yes it is. Thanks for your patient review!
#216) * add NavbarToggler example * feat(Collapse): add Collapse component #79 (#201) * init collapse * add collapse animation * add margin between toggle button and collapse * disable lint on force refresh DOM line. * add test to Collapse * remove height after shown * Revert "remove height after shown" This reverts commit eff9353. * remove height after shown. * add more test * use setState() to set inline height style * remove custom tag in doc * add inline style test * remove comment * set height to null when isOpen is true * add initial state test * chore(release): adding 3.8.0 (#217) * chore(release): adding 3.8.0 * feat(Collapse): add cssModule support * feat(refs): add getRef to focusable components (#218) * chore(release): adding 3.8.1 (#219) * add NavbarToggler example * update example to use Collapse component
2 participants
No description provided.