Skip to content
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

Refactor devicelist page #435

Merged
merged 20 commits into from
Apr 2, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions frontend/src/devicelist/DeviceListPage.jsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import React from "react";

import Page from "../general/Page";
import DeviceListTabTable from "./DeviceListTabTable";
import * as useStyles from "../DefaultMakeStylesTheme";
import * as dataSource from "../api/DeviceApi";
import DeviceListPageContents from "./DeviceListPageContents";

export default function DeviceListPage() {
const breadcrumb = [
Expand All @@ -12,7 +10,7 @@ export default function DeviceListPage() {
];
return (
<Page title="My Devices" breadcrumbs={breadcrumb} deviceList>
<DeviceListTabTable classes={useStyles} dataSource={dataSource} />
<DeviceListPageContents />
</Page>
);
}
66 changes: 66 additions & 0 deletions frontend/src/devicelist/DeviceListPageContents.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import React from "react";

import { getSenders, getReceivers } from "../api/DeviceApi";
import DeviceTableTitle from "./DeviceTableTitle";
import DevicesTable from "./DevicesTable";

export default class DeviceListPageContents extends React.Component {
constructor(props) {
super(props);
this.state = {
selected: 0,
senders: [],
receivers: []
};

this.handleChange = this.handleChange.bind(this);
this.handleSendersChange = this.handleSendersChange.bind(this);
this.handleReceiversChange = this.handleReceiversChange.bind(this);
}

componentDidMount() {
getSenders(this.handleSendersChange);
getReceivers(this.handleReceiversChange);
}

handleChange(value) {
this.setState({
selected: value
});
}

handleSendersChange(senders) {
this.setState({
senders
});
}

handleReceiversChange(receivers) {
this.setState({
receivers
});
}

getDevices() {
const { receivers, senders, selected } = this.state;
switch (selected) {
case 1:
return senders;
case 2:
return receivers;
default:
return senders.concat(receivers);
}
}

getTitle() {
const { selected } = this.state;
return (
<DeviceTableTitle index={selected} handleChange={this.handleChange} />
);
}

render() {
return <DevicesTable devices={this.getDevices()} title={this.getTitle()} />;
}
Comment on lines +56 to +65
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose to inject the entire title component into DevicesTable rather than just have it in DevicesTable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes it read cleaner and is easier to test

}
78 changes: 0 additions & 78 deletions frontend/src/devicelist/DeviceListTabTable.jsx

This file was deleted.

38 changes: 38 additions & 0 deletions frontend/src/devicelist/DeviceTableTitle.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import React from "react";
import PropTypes from "prop-types";
import { MenuItem, Select } from "@material-ui/core";
import StyledInput from "./StyledInput";

export default class DeviceTableTitle extends React.Component {
constructor(props) {
super(props);

this.handleChange = this.handleChange.bind(this);
}

handleChange(event) {
const { handleChange } = this.props;
handleChange(event.target.value);
}

render() {
const { index } = this.props;
return (
<Select
id="device-table-title-select"
value={index}
onChange={this.handleChange}
input={<StyledInput />}
>
<MenuItem value={0}>All Devices</MenuItem>
<MenuItem value={1}>Senders</MenuItem>
<MenuItem value={2}>Receivers</MenuItem>
</Select>
);
}
}

DeviceTableTitle.propTypes = {
index: PropTypes.number.isRequired,
handleChange: PropTypes.func.isRequired
};
2 changes: 1 addition & 1 deletion frontend/src/devicelist/DevicesTable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,6 @@ export default function DevicesTable(props) {
}

DevicesTable.propTypes = {
title: PropTypes.string.isRequired,
title: PropTypes.node.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERY SEXY that all you had to change in this file was this!!! \o/

devices: PropTypes.arrayOf(PropTypes.instanceOf(DeviceInfo)).isRequired
};
18 changes: 18 additions & 0 deletions frontend/src/devicelist/StyledInput.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { InputBase, withStyles } from "@material-ui/core";

export default withStyles({
input: {
borderRadius: 4,
position: "relative",
fontWeight: 500,
letterSpacing: "0.0075em",
lineHeight: "1.6",
fontSize: "1.25rem",
padding: "10px 26px 10px 12px",
"&:focus": {
borderRadius: 4,
borderColor: "#80bdff",
boxShadow: "0 0 0 0.2rem rgba(0,123,255,.25)"
}
}
})(InputBase);
27 changes: 0 additions & 27 deletions frontend/src/devicelist/TabPanel.jsx

This file was deleted.

6 changes: 3 additions & 3 deletions frontend/src/devicelist/__tests__/DeviceListPage.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { beforeEach, describe, expect } from "@jest/globals";

import DeviceListPage from "../DeviceListPage";
import Page from "../../general/Page";
import DeviceListTabTable from "../DeviceListTabTable";
import DeviceListPageContents from "../DeviceListPageContents";

Enzyme.configure({ adapter: new Adapter() });

Expand All @@ -30,8 +30,8 @@ describe("<DeviceListPage/> functional component", () => {
expect(page.props().breadcrumbs).toStrictEqual(expectedBreadcumbs);
expect(page.props().deviceList).toBe(true);
});
it("Contains 1 <DeviceListTabTable/> component", () => {
expect(wrapper.find(DeviceListTabTable)).toHaveLength(1);
it("Contains 1 <DeviceListPageContents/> component", () => {
expect(wrapper.find(DeviceListPageContents)).toHaveLength(1);
});
});
});
Loading