implement add / rename / delete column in data browser [axot: 51]#9685
Conversation
037fa4d to
2fc6257
Compare
2fc6257 to
cecb83a
Compare
|
Hey @islemaster , would you mind taking a look at some of the latest data browser changes? I'm happy to give you an overview before you dive in if that would be helpful. |
713dfe5 to
5ac6ca8
Compare
5ac6ca8 to
6af308e
Compare
| ); | ||
| } | ||
| }); | ||
| export default Radium(AddColumnButton); |
There was a problem hiding this comment.
Do we expect this add button to change much? If not, I think there's a more idiomatic way to define it. You're not actually using Radium or redux, the whole thing is stateless, etc. Any reason it shouldn't just be expressed this way?
import React from 'react';
import {button as buttonStyle} from './dataStyles';
const containerStyle = {
float: 'right',
paddingTop: 10,
paddingLeft: 10,
paddingBottom: 10,
paddingRight: 0
};
export default function AddColumnButton(props) {
return (
<div style={containerStyle}>
<button className="btn" onClick={props.addColumn} style={dataStyles.button}>
Add column
</button>
</div>
);
}
AddColumnButton.propTypes = {
addColumn: React.PropTypes.func.isRequired
};There was a problem hiding this comment.
Hi Brad, I agree that this react class is a little light right now. However, the next feature on my plate is to add Import, Export and Delete buttons to this same row, and I had secretly been planning to rename this class to contain the entire top row of buttons (import, export, add column, delete table):
I've renamed the file and removed some cruft.
|
|
||
| componentDidMount() { | ||
| if (this.props.isEditing) { | ||
| ReactDOM.findDOMNode(this.refs.input).select(); |
There was a problem hiding this comment.
Because this.refs.input is a DOM component and not a composite component, ReactDOM.findDOMNode isn't necessary here (as of React 0.14.0).
Also, string refs are considered legacy; our styleguide hasn't deprecated them yet, but did you consider callback refs?
|
This looks great Dave! Nice clean implementation. It looks like you're way past this being useful, but it might be good reference: I'm trying to use Reactabular.js for the NetSim Log Table. Even so I'm wrestling with some layout stuff; maybe you can review it when it's ready? The stateful editing stuff in ColumnHeader might be complex enough to merit a unit test. Overall LGTM though. Nice work! |
|
Thanks for the detailed review, Brad! It's great to learn about some of these best practices. I've addressed all feedback in code unless noted otherwise comments here. I'd be happy to review the Reactabular change -- I haven't invested too much in HTML tables here, so it'll be a good way to get a sense of whether it'll be useful for me here when I go to implement sorting and such. |
|
Agreed the column header state needs tests -- noted in https://codeorg.axosoft.com/viewitem?id=59&type=features&force_use_number=true . |
axosoft work item: https://codeorg.axosoft.com/viewitem?id=51&type=features&force_use_number=true
spec: https://docs.google.com/document/d/1UfBgHCisr_8jSt8zHFgJL-XrlLthTrXJQZJwoedGYg4/edit# --> search for "Adding a column"
demo:
