-
Notifications
You must be signed in to change notification settings - Fork 479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Write 'move students' functionality in React #22060
Changes from 14 commits
09fb3fb
de51cf0
ec68f8d
9a8146f
dac5bec
e229df7
a93c577
72d0fb8
c726828
68fe965
9aee47c
51edc25
3ce93d7
bb9e58e
b0a9d5c
10397b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,223 @@ | ||
import React, {Component, PropTypes} from 'react'; | ||
import i18n from "@cdo/locale"; | ||
import {Table, sort} from 'reactabular'; | ||
import wrappedSortable from '../tables/wrapped_sortable'; | ||
import {tableLayoutStyles, sortableOptions} from "../tables/tableConstants"; | ||
import Immutable from 'immutable'; | ||
import orderBy from 'lodash/orderBy'; | ||
import Button from '../Button'; | ||
import BaseDialog from '../BaseDialog'; | ||
import DialogFooter from "../teacherDashboard/DialogFooter"; | ||
|
||
const PADDING = 20; | ||
const TABLE_WIDTH = 300; | ||
const CHECKBOX_CELL_WIDTH = 50; | ||
|
||
const styles = { | ||
dialog: { | ||
paddingLeft: PADDING, | ||
paddingRight: PADDING, | ||
paddingTop: PADDING, | ||
paddingBottom: PADDING | ||
}, | ||
table: { | ||
width: TABLE_WIDTH | ||
}, | ||
checkboxCell: { | ||
width: CHECKBOX_CELL_WIDTH, | ||
textAlign: 'center' | ||
}, | ||
checkbox: { | ||
margin: 0 | ||
} | ||
}; | ||
|
||
class MoveStudents extends Component { | ||
static propTypes = { | ||
studentData: PropTypes.array.isRequired | ||
}; | ||
|
||
state = { | ||
isDialogOpen: false, | ||
selectedIds: [] | ||
}; | ||
|
||
openDialog = () => { | ||
this.setState({isDialogOpen: true}); | ||
}; | ||
|
||
closeDialog = () => { | ||
this.setState({ | ||
isDialogOpen: false, | ||
selectedIds: [] | ||
}); | ||
}; | ||
|
||
getStudentIds = () => { | ||
return this.props.studentData.map(s => s.id); | ||
}; | ||
|
||
areAllSelected = () => { | ||
return Immutable.Set(this.state.selectedIds).isSuperset(this.getStudentIds()); | ||
}; | ||
|
||
toggleSelectAll = () => { | ||
if (this.areAllSelected()) { | ||
this.setState({selectedIds: []}); | ||
} else { | ||
this.setState({selectedIds: this.getStudentIds()}); | ||
} | ||
}; | ||
|
||
toggleStudentSelected = (studentId) => { | ||
let selectedIds = [...this.state.selectedIds]; | ||
|
||
if (this.state.selectedIds.includes(studentId)) { | ||
const studentIndex = selectedIds.indexOf(studentId); | ||
selectedIds.splice(studentIndex, 1); | ||
} else { | ||
selectedIds.push(studentId); | ||
} | ||
|
||
this.setState({selectedIds}); | ||
}; | ||
|
||
selectedStudentHeaderFormatter = () => { | ||
return ( | ||
<input | ||
style={styles.checkbox} | ||
type="checkbox" | ||
checked={this.areAllSelected()} | ||
onChange={this.toggleSelectAll} | ||
/> | ||
); | ||
}; | ||
|
||
selectedStudentFormatter = (_, {rowData}) => { | ||
const isChecked = this.state.selectedIds.includes(rowData.id); | ||
|
||
return ( | ||
<input | ||
style={styles.checkbox} | ||
type="checkbox" | ||
checked={isChecked} | ||
onChange={() => this.toggleStudentSelected(rowData.id)} | ||
/> | ||
); | ||
}; | ||
|
||
getColumns = (sortable) => { | ||
return [{ | ||
property: 'selected', | ||
header: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some weird indentation here, and below. Maybe: return [
{
property: 'selected',
header: { |
||
label: '', // TODO: what should this label be?, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it needs a label - can remove this TODO. |
||
format: this.selectedStudentHeaderFormatter, | ||
props: { | ||
style: { | ||
...tableLayoutStyles.headerCell, | ||
...styles.checkboxCell | ||
}}, | ||
}, | ||
cell: { | ||
format: this.selectedStudentFormatter, | ||
props: { | ||
style: { | ||
...tableLayoutStyles.cell, | ||
...styles.checkboxCell | ||
}} | ||
} | ||
}, | ||
{ | ||
property: 'name', | ||
header: { | ||
label: i18n.name(), | ||
props: { | ||
style: { | ||
...tableLayoutStyles.headerCell, | ||
}}, | ||
transforms: [sortable], | ||
}, | ||
cell: { | ||
props: { | ||
style: { | ||
...tableLayoutStyles.cell | ||
}} | ||
} | ||
}]; | ||
}; | ||
|
||
getSortingColumns = () => { | ||
return this.state.sortingColumns || {}; | ||
}; | ||
|
||
// The user requested a new sorting column. Adjust the state accordingly. | ||
onSort = (selectedColumn) => { | ||
this.setState({ | ||
sortingColumns: sort.byColumn({ | ||
sortingColumns: this.state.sortingColumns, | ||
// Custom sortingOrder removes 'no-sort' from the cycle | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use this in eleven places now. Let's extract it somewhere! Maybe into |
||
sortingOrder: { | ||
FIRST: 'asc', | ||
asc: 'desc', | ||
desc: 'asc' | ||
}, | ||
selectedColumn | ||
}) | ||
}); | ||
}; | ||
|
||
sortRows = (data, columnIndexList, orderList) => { | ||
return orderBy(data, columnIndexList, orderList); | ||
}; | ||
|
||
render() { | ||
// Define a sorting transform that can be applied to each column | ||
const sortable = wrappedSortable(this.getSortingColumns, this.onSort, sortableOptions); | ||
const columns = this.getColumns(sortable); | ||
const sortingColumns = this.getSortingColumns(); | ||
|
||
const sortedRows = sort.sorter({ | ||
columns, | ||
sortingColumns, | ||
sort: this.sortRows, | ||
})(this.props.studentData); | ||
|
||
return ( | ||
<div> | ||
<Button | ||
onClick={this.openDialog} | ||
color={Button.ButtonColor.gray} | ||
text={i18n.moveStudents()} | ||
/> | ||
<BaseDialog | ||
useUpdatedStyles | ||
isOpen={this.state.isDialogOpen} | ||
style={styles.dialog} | ||
handleClose={this.closeDialog} | ||
> | ||
<Table.Provider | ||
columns={columns} | ||
style={styles.table} | ||
> | ||
<Table.Header /> | ||
<Table.Body rows={sortedRows} rowKey="id" /> | ||
</Table.Provider> | ||
<DialogFooter> | ||
<Button | ||
text={i18n.dialogCancel()} | ||
onClick={this.closeDialog} | ||
color={Button.ButtonColor.gray} | ||
/> | ||
<Button | ||
text={i18n.moveStudents()} | ||
onClick={() => {}} | ||
color={Button.ButtonColor.orange} | ||
/> | ||
</DialogFooter> | ||
</BaseDialog> | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
export default MoveStudents; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import React from 'react'; | ||
import MoveStudents from './MoveStudents'; | ||
|
||
const studentData = [ | ||
{ | ||
id: 1, | ||
name: 'Student A' | ||
}, | ||
{ | ||
id: 3, | ||
name: 'Student C' | ||
}, | ||
{ | ||
id: 2, | ||
name: 'Student B' | ||
} | ||
]; | ||
|
||
export default storybook => { | ||
storybook | ||
.storiesOf('MoveStudents', module) | ||
.addStoryTable([ | ||
{ | ||
name: 'Move students dialog', | ||
description: 'Ability to move students in a certain section to a different section or teacher', | ||
story: () => ( | ||
<MoveStudents studentData={studentData} /> | ||
) | ||
} | ||
]); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
import React from 'react'; | ||
import {shallow} from 'enzyme'; | ||
import {expect} from '../../../util/configuredChai'; | ||
import MoveStudents from '@cdo/apps/templates/manageStudents/MoveStudents'; | ||
|
||
const studentData = [ | ||
{id: 1, name: 'studentb'}, | ||
{id: 3, name: 'studenta'}, | ||
{id: 0, name: ''}, | ||
{id: 2, name: 'studentf'} | ||
]; | ||
|
||
describe('MoveStudents', () => { | ||
let wrapper; | ||
|
||
beforeEach(() => { | ||
wrapper = shallow(<MoveStudents studentData={studentData}/>); | ||
}); | ||
|
||
describe('#openDialog', () => { | ||
it('sets isDialogOpen state to true', () => { | ||
wrapper.instance().openDialog(); | ||
expect(wrapper.instance().state.isDialogOpen).to.equal(true); | ||
}); | ||
}); | ||
|
||
describe('#closeDialog', () => { | ||
it('sets isDialogOpen state to false', () => { | ||
wrapper.instance().closeDialog(); | ||
expect(wrapper.instance().state.isDialogOpen).to.equal(false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😆 Here you've tested that the dialog stays closed when it's already closed and you call This is also interesting because in your implementation those two cases are the same code path - but I could imagine a world where some future developer refactors this component and breaks one case but not the other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😱 copy-pasting tests wins again |
||
}); | ||
|
||
it('clears selectedIds in state', () => { | ||
wrapper.instance().state.selectedIds = [1,2]; | ||
expect(wrapper.instance().state.selectedIds).to.have.members([1,2]); | ||
wrapper.instance().closeDialog(); | ||
expect(wrapper.instance().state.selectedIds).to.have.members([]); | ||
}); | ||
}); | ||
|
||
describe('#getStudentIds', () => { | ||
it('returns all student ids', () => { | ||
expect(wrapper.instance().getStudentIds()).to.have.members([0,1,2,3]); | ||
}); | ||
}); | ||
|
||
describe('#areAllSelected', () => { | ||
it('returns true if all student ids are in selectedIds', () => { | ||
wrapper.instance().state.selectedIds = [0,1,2,3]; | ||
expect(wrapper.instance().areAllSelected()).to.equal(true); | ||
}); | ||
|
||
it('returns false if all student ids are not in selectedIds', () => { | ||
wrapper.instance().state.selectedIds = [0,1,2]; | ||
expect(wrapper.instance().areAllSelected()).to.equal(false); | ||
}); | ||
}); | ||
|
||
describe('#toggleSelectAll', () => { | ||
it('clears selectedIds in state if all ids are selected', () => { | ||
wrapper.instance().state.selectedIds = [0,1,2,3]; | ||
wrapper.instance().toggleSelectAll(); | ||
expect(wrapper.instance().state.selectedIds).to.have.members([]); | ||
}); | ||
|
||
it('adds all ids to selectedIds in state if some ids are selected', () => { | ||
wrapper.instance().state.selectedIds = [0,1]; | ||
wrapper.instance().toggleSelectAll(); | ||
expect(wrapper.instance().state.selectedIds).to.have.members([0,1,2,3]); | ||
}); | ||
|
||
it('adds all ids to selectedIds in state if no ids are selected', () => { | ||
wrapper.instance().state.selectedIds = []; | ||
wrapper.instance().toggleSelectAll(); | ||
expect(wrapper.instance().state.selectedIds).to.have.members([0,1,2,3]); | ||
}); | ||
}); | ||
|
||
describe('#toggleStudentSelected', () => { | ||
it('removes student id from selectedIds in state if already present', () => { | ||
wrapper.instance().state.selectedIds = [1]; | ||
wrapper.instance().toggleStudentSelected(1); | ||
expect(wrapper.instance().state.selectedIds).to.have.members([]); | ||
}); | ||
|
||
it('adds student id to selectedIds in state if not already present', () => { | ||
wrapper.instance().state.selectedIds = [1]; | ||
wrapper.instance().toggleStudentSelected(0); | ||
expect(wrapper.instance().state.selectedIds).to.have.members([0,1]); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd specify exactly what you need in the shape for
studentData
: