Section table react #15745
Section table react #15745
Conversation
|
|
||
| this.onClickDelete = this.onClickDelete.bind(this); | ||
| this.onClickDeleteNo = this.onClickDeleteNo.bind(this); | ||
| this.onClickDeleteYes = this.onClickDeleteYes.bind(this); |
There was a problem hiding this comment.
In the past someone (I think @pcardune?) suggested I use property initializer syntax and arrow functions instead of self-binding in the constructor, e.g.
export default class SectionRow extends Component {
onClickDelete = () => {
this.setState({deleting: true});
};
}It's clever. Thoughts?
There was a problem hiding this comment.
That is clever. I think I like it if it works.
There was a problem hiding this comment.
I learned everything from this blog post: https://babeljs.io/blog/2015/06/07/react-on-es6-plus
| this.onClickDeleteYes = this.onClickDeleteYes.bind(this); | ||
|
|
||
| this.state = { | ||
| editing: false, |
There was a problem hiding this comment.
Looks like state.editing is currently unused.
There was a problem hiding this comment.
Yeah, this will be toggled when you click the Edit button (but that work has not yet been done)
| )} | ||
| </div> | ||
| )} | ||
| {this.state.deleting && ( |
There was a problem hiding this comment.
It might be nice to extract the buttons here into a functional stateless component in this file. The {!this.state.deleting && and {this.state.deleting && side-by-side subtly suggest four states to me, where you could use a proper if/else in a subcomponent. It also seems likely that this will get more complex as we add the editing states, could extract it then.
| <col/> | ||
| <col/> | ||
| <col/> | ||
| </colgroup> |
There was a problem hiding this comment.
Not totally clear what we're doing with colgroup here, but I think we either need to add two <col/> to bring the total up to 9, or remove the tags that don't set a width because they're no-ops right now.
| </colgroup> | ||
| <tbody> | ||
| <tr> | ||
| <th style={styles.headerRow}> |
There was a problem hiding this comment.
Why isn't our header row in thead?
There was a problem hiding this comment.
Copied this from the haml. Not sure why it wasnt in the head there. Changing the look of this header row is one of the future work items needed. I will look to move it then.
Bjvanminnen
force-pushed
the
sectionTableReact
branch
from
d21a286
to
b1d007e
Compare
| } | ||
| }; | ||
|
|
||
| export default class SectionTable extends Component { |
There was a problem hiding this comment.
Nothing yet. Was planning to do that in sync with adding tests as one of my next steps.
| export const sectionShape = PropTypes.shape({ | ||
| id: PropTypes.number.isRequired, | ||
| name: PropTypes.string.isRequired, | ||
| loginType: PropTypes.oneOf(['word', 'email', 'picture']).isRequired, |
There was a problem hiding this comment.
Worth making this an enum const somewhere?
There was a problem hiding this comment.
This is the only place this is used (at least on the client) so far. If it ends up being used elsewhere, I will consider an enum.
| {section.stageExtras ? i18n.yes() : i18n.no()} | ||
| </td> | ||
| <td> | ||
| {section.pairingAllowed ? i18n.yes() : i18n.no()} |
This is the beginning of the work to convert our Sections table to React. It provides a flag (
reactSections) which enabled shows this new table instead of the old one. With the flag off, everything should be identical.It is still very early progress with big pieces missing, but I wanted to have a series of smaller commits instead of one large one. Some big pieces missing