[sqllab] slide animations when adding/removing/toggling TableElement #1472
Conversation
|
would be great to see screenshots/gif of this ui change! |
| <i className="fa fa-i-cursor" /> rename tab | ||
| </MenuItem> | ||
| <MenuItem eventKey="3"> | ||
| {qe && <MenuItem eventKey="3"> |
There was a problem hiding this comment.
i think this is more readable when the condition and component are on separate lines:
{qe &&
<MenuItem eventKey="3">
<i className="fa fa-clipboard" /> <CopyQueryTabUrl queryEditor={qe} />
</MenuItem>
}
| tooltip="Data preview" | ||
| href="#" | ||
| /> | ||
| {table.selectStar && <CopyToClipboard |
There was a problem hiding this comment.
move <CopyToClipboard to next line plz.
| expect(React.isValidElement(<CopyQueryTabUrl />)).to.equal(true); | ||
| }); | ||
| it('renders with props', () => { | ||
| it('be valid with props', () => { |
| } | ||
|
|
||
| render() { | ||
| getHeader() { |
There was a problem hiding this comment.
for any method that returns a node/element it's a nice pattern to name them with render, so this would be renderHeader,
There was a problem hiding this comment.
could this be pulled out into it's own component called <TableHeader />, nice to break things into smaller components, makes things more easily testable.
| const cols = table.columns.slice(); | ||
| return header; | ||
| } | ||
| getMetadata() { |
| </a> | ||
| </div> | ||
| <div className="pull-right"> | ||
| <ButtonGroup className="ws-el-controls pull-right"> |
There was a problem hiding this comment.
would be nice to pull this out into a component called <TableControls />
|
@ascott I addressed all the comments except the ones calling to refactor out smaller components. I understand the value of doing that, but I'd rather keep it out of the scope of this PR. |
|
LGTM 🚢 |
mistercrunch
added
🏷️ bot
2 participants
@ascott @vera-liu @bkyryliuk
Added a
mockedActionsin fixtures that uses thesinonlib, makes it easy to test if actions are triggered as they should.To test outcomes after animations I decided to define
timeoutas a prop, which can be set to zero in the context of a test, using a short delay (using setTimeout) was still necessary.