Implement table list controls in data browser#9374
Conversation
… we want dialogs instead This reverts commit e4af5ec.
| style={dataStyles.input} | ||
| placeholder={msg.dataTableNamePlaceholder()} | ||
| value={this.state.newTableName} | ||
| onChange={this.handleChange}/> |
There was a problem hiding this comment.
nit: attributes should be indented 4 spaces instead of 2 here and elsewhere
| // when the the first record is added to it. | ||
| dispatch(addTableName(tableName)); | ||
|
|
||
| dispatch(changeView(DataView.TABLE, tableName)); |
There was a problem hiding this comment.
Not too sure what I think about having our mapDispatchToProps method here creating a prop that dispatches two actions. Seems like should either have a single tableAdd action that takes care of everything, or we should have two props, and callers in our component call both this.addTableName and this.changeView.
My understanding of this method mapDispatchToProps (i.e. the second param passed to connect) is that it's mostly just to make it so that our component doesnt need to be calling dispatch directly. Additional complexity should be avoided (though you could make the case that this is still a small enough amount of complexity).
|
I've addressed all feedback. Please take another look |
| case DELETE_TABLE_NAME: | ||
| map = Object.assign({}, state.tableListMap); | ||
| delete map[action.tableName]; | ||
| return state.set('tableListMap', map); |
There was a problem hiding this comment.
I wonder if we could/should do the same sort of thing here and just set map[action.tableName] to undefined. Up to you.
return state.set('tableListMap', Object.assign({}, state.tableListMap, {
[action.tableName]: undefined
}));
There was a problem hiding this comment.
This is a bit of an air bubble under the wall paper. Setting a key to undefined won't prevent it from showing up in Object.keys:
var obj = {foo: 'bar'}
undefined
obj.foo = undefined
undefined
Object.keys(obj)
["foo"]
so this would push a bit of added complexity into the render method of the DataOverview to check that the keys are truthy (not just present). To clean this up a bit anyway, I've added a block around the inside of the case statement, so that I can move the let there without eslint complaining.
|
lgtm. it's great to see this progressing :) |
08c2138 to
2613e41
Compare
| let map = Object.assign({}, state.tableListMap); | ||
| delete map[action.tableName]; | ||
| return state.set('tableListMap', map); | ||
| } |
There was a problem hiding this comment.
the arbitrary block is valid syntax? i thought i'd tried this before and found that it wasnt. maybe i did something wrong
There was a problem hiding this comment.
I'm not sure if they are allowed to be arbitrary, but they appear to be allowed (and encouraged by airbnb style guide) for switch/case statements: https://github.com/airbnb/javascript#comparison--switch-blocks
This PR mostly finishes the UI for the table list (aka Overview) in the data browser. The one exception is the modal experience (as shown in the spec) which currently uses
window.confirmas a placeholder. Getting this modal experience right is going to be a bit of work, so I'll do it next in its own PR.screencap:
