Provisioners, Worker Types, Workers, Worker #65

helfi92 · 2018-04-19T18:35:00Z

Resolves #15
Resolves #16
Resolves #18
Resolves #17

Implemented so that we have consistent dropdowns across the site

A similar implementation to the ConnectionDataTable except that it takes raw data and no pagination

`QuarantineButton` was created mainly because it has somewhat a complex dialog where a user can modify the quarantineUntil date. To isolate the state management as well as possible code duplication, this component was created.

This component integrates gracefully in a TableCell. It aligns list items with column headers along with other styling which otherwise needs to be duplicated in every component that would use a `ListItem` inside a `TableCell`.

Done in order to be consistent with the worker types view

helfi92 · 2018-04-19T18:38:07Z

View Provisioners

View Worker Types

View Workers

View Worker

eliperelman

So far this is looking pretty hot. In the first round here, I'd also like to request some UI changes:

In the provisioners card list, use the same style I have for the hovering list item link icons as I do in the task view, so they are faded normally, but are bright on hover. Feel free to extract into a mixin in theme.js if it makes sense.
On the Worker Types view, remove the paper/card wrapper from the select box. Experiment with positioning the select on the right.
On the Workers view, remove the paper/card wrapper around the select box. Experiment with position the select on the right. If there are any actions, do not render them here at the top, but try using the SpeedDial component.
On the Worker view, remove the paper/card wrapper surrounding that metadata. We can probably remove the words "Worker Details". Move the actions to a Speed Dial.
For the slide-up task metadata, can you make that come in from the right instead of the bottom?

eliperelman · 2018-04-21T13:26:34Z

src/components/ProvisionerDetailsCard/index.jsx

+export default class ProvisionerDetailsCard extends Component {
+  static propTypes = {
+    /** A `react-router-dom` history object */
+    history: object,


Instead of passing history as a prop from the parent, let's use the withRouter decorator.

import { withRouter } from 'react-router-dom'; @withRouter export default class ProvisionerDetailsCard extends Component {

eliperelman · 2018-04-21T13:27:19Z

src/components/ProvisionerDetailsCard/index.jsx

+    /** A `react-router-dom` history object */
+    history: object,
+    /** A GraphQL provisioner response. */
+    provisioner,


This should be required.

eliperelman · 2018-04-21T13:30:48Z

src/components/ProvisionerDetailsCard/index.jsx

+              <ListItemText primary="Description" />
+              {showDescription ? <ChevronUpIcon /> : <ChevronDownIcon />}
+            </ListItem>
+            <Collapse in={showDescription} timeout="auto" unmountOnExit>


It looks like it's possible to not have a description. So let's remove the chevron icons when the description is not there and render the n/a, otherwise render the collapse.

Agreed. This will, however, require setting a min-height on the card in order to have height consistency with the other cards which should be ok IMO.

eliperelman · 2018-04-21T13:32:58Z

src/components/QuarantineButton/index.jsx

+        title="Quarantine?"
+        body={
+          <Fragment>
+            <Markdown>


Remove this from being Markdown and just use the name of the property as "Quarantine Until".

eliperelman · 2018-04-21T13:33:11Z

src/components/QuarantineButton/index.jsx

+            <Markdown>
+              Quarantining a worker allows the machine to remain alive but not
+              accept jobs. Note that a quarantine can be lifted by setting
+              `quarantineUntil` to the present time (or somewhere in the past).


Remove parentheses from text.

eliperelman · 2018-04-21T14:20:11Z

src/views/Provisioners/ViewWorkers/index.jsx

+      data: { fetchMore },
+    } = this.props;
+
+    return fetchMore({


In the future it looks like we can make this more generic in a util.

eliperelman · 2018-04-21T14:21:09Z

src/views/Provisioners/ViewWorkers/index.jsx

+
+    return (
+      <Dashboard
+        title="View Workers"


Make this just "Workers".

eliperelman · 2018-04-21T14:22:32Z

src/views/Provisioners/ViewWorkers/index.jsx

+                </CardContent>
+              </Card>
+              <br />
+              {loading ? (


Why the loading check here? The connection table already has a loading indicator.

eliperelman · 2018-04-21T14:23:10Z

src/views/Provisioners/index.jsx

+    } = this.props;
+
+    return (
+      <Switch>


eliperelman · 2018-04-21T14:23:24Z

src/views/Provisioners/index.jsx

+);
+
+@hot(module)
+export default class Task extends Component {


Rename to Provisioners or something.

eliperelman · 2018-04-24T11:52:45Z

src/components/ButtonDrawer/index.jsx

+ * A  button that displays a drawer when clicked.
+ * The drawer opens above all other content until a selection is selected.
+ */
+export default class ButtonDrawer extends PureComponent {


I'm not sure the value of this component. Since it's completely controlled by the parent, and doesn't have any additional functionality or styles, I think it's OK to inline these into their parent component.

eliperelman · 2018-04-24T11:53:47Z

src/components/SpeedDial/index.jsx

@@ -11,7 +12,7 @@ const IS_TOUCH = 'ontouchstart' in document.documentElement;
 @withStyles(theme => ({
  speedDial: {
    position: 'absolute',
-    bottom: theme.spacing.double,
+    top: theme.spacing.double,


I want these to stay at the bottom. The log view doesn't use this component and it isn't a speed dial, so I want to be consistent with material.

eliperelman · 2018-04-24T11:59:16Z

src/components/SpeedDialActionDialog/index.jsx

+/**
+ * A SpeedDialAction that displays a dialog when clicked.
+ */
+export default class SpeedDialogAction extends PureComponent {


I don't think it makes sense to combine these components. The button can be inlined into its parent, and the "async action" dialog can be its own component.

eliperelman · 2018-04-24T12:01:50Z

src/components/SpeedDialActionDialog/index.jsx

+          onClick={this.handleOpen}
+          {...props}
+        />
+        <Dialog open={open} onClose={this.handleClose}>


https://material-ui-next.com/demos/dialogs/#responsive-full-screen

eliperelman · 2018-04-24T12:10:37Z

src/components/ConnectionDataTable/index.jsx

-                page={page}
-                onChangePage={this.handlePageChange}
-              />
+      <div>


Why multiple divs instead of a Fragment? Why are we rendering 2 tables when loading/why are we changing the existing loading layout?

@eliperelman The outermost div can definitely be a Fragment but the second div is used to keep the table footer from scrolling with the table. To have TablePagination not scroll when the table is small as well as the arrows not positioned at the end of the scroll, it needs to be outside the Table component (see example in material docs). Next, to display the spinner, a Table with a TableFooter is created in order to have the spinner positioned at the same spot as the arrows in TablePagination.

To stop rendering 2 tables, the second Table can be replaced with:

<div className={classes.spinner}> <Spinner size={24} /> </div>

spinner: { height: 56, minHeight: 56, paddingRight: 2, display: 'flex', alignItems: 'center', flexDirection: 'row-reverse', },

I think ^ makes more sense to use.

eliperelman

Round 2. For UI changes:

Upon seeing the right drawer, I think it's overkill. We are already linking to the task from the Worker table, and that has the information about the task, so the drawer is redundant. I think we should kill this and just link to the task.
I mentioned it in the comments, but let's use the Speed Dial component as intended by material-ui, and place it in the lower right, with sub-actions appearing on hover.
Can you make the icons that appear in the table to be a little smaller? The default of 24 seems a bit big, so maybe 16-20?

eliperelman · 2018-04-24T12:15:54Z

src/components/ConnectionDataTable/index.jsx

+                      <TableSortLabel
+                        active={header === sortByHeader}
+                        direction={sortDirection || 'desc'}
+                        onClick={() => this.handleHeaderClick(header)}>


Can we make this passed by reference, and use the target/currentTarget ID to drive the passed value?

handleHeaderClick = e => { this.props.onHeaderClick(e.target.id); }; id={header} onClick={this.props.onHeaderClick && this.handleHeaderClick}

We could, but I think we should (when we can) try to use IDs for anchors only. We could rely on e.target.textContent instead, if that's ok with you.

try to use IDs for anchors only

Why?

Sorry, by anchors I mean things like toolbar, header, footer, etc. I would typically use ids as fragment identifier to jump to anchors. There's also the specificity war that can happen with CSS. I don't think it's the end of the world if we use an id here but using something like textContent will avoid using an id and is altogether less code.

Let me address the points:

fragment identifier to jump to anchors

We don't use this feature since we aren't really document-based but app and JS driven. It's useful in docs, though, but in there we won't be doing a bunch of event bindings.

There's also the specificity war that can happen with CSS

We aren't using CSS, nor do we do any CSS bindings based on IDs.

using something like textContent

While this would work, this relies on the item's representation in the DOM vs. our own identifier, which may not always align. Best to be explicit.

eliperelman · 2018-04-24T12:16:39Z

src/components/DataTable/index.jsx

+
+@withStyles({
+  tableWrapper: {
+    overflowX: 'auto',


Will this be resolved by mui/material-ui#11062?

Weird, it seems to be working now with our current 1.0.0-beta.41. I'll just remove it.

eliperelman · 2018-04-24T12:17:19Z

src/components/DataTable/index.jsx

+                    <TableSortLabel
+                      active={header === sortByHeader}
+                      direction={sortDirection || 'desc'}
+                      onClick={() => this.handleHeaderClick(header)}>


Same concern about passing by reference.

eliperelman · 2018-04-24T12:21:15Z

src/components/DialogAction/index.jsx

+/**
+ * A button that displays a dialog when clicked.
+ */
+export default class DialogAction extends PureComponent {


Split this component up. The button should live in the parent component, and the "async action dialog" can be its own component.

This needs to be a Component since the props can't be shallowly compared.

eliperelman · 2018-04-24T12:22:58Z

src/components/Dropdown/index.jsx

+    } = this.props;
+
+    return (
+      <FormControl className={classes.root} {...props}>


Why is this preferred over <TextField select />?

eliperelman · 2018-04-24T14:19:12Z

src/components/WorkerTypesTable/index.jsx

+        awsProvisionerWorkerTypeSummaries,
+      } = this.props;
+      const sortByProperty = camelCase(sortBy);
+      // Normalize worker types for aws-provisioner-v1


Do you think this normalization should happen on the server, or in a util function?

I think a util function makes more sense here. We have 14 provisioners and this normalization is only for aws-provisioner.

eliperelman · 2018-04-24T14:22:28Z

src/components/WorkersTable/index.jsx

+  static propTypes = {
+    /** Workers GraphQL PageConnection instance. */
+    workersConnection: shape({
+      edges: array,


Can this be typed stronger?

eliperelman · 2018-04-24T14:22:49Z

src/components/WorkersTable/index.jsx

+
+  createSortedWorkersConnection = memoizeWith(
+    () =>
+      `${toString(this.props.workersConnection.edges)}-${this.state.sortBy}-${


See other comment about calculating this.

eliperelman · 2018-04-24T14:42:40Z

src/views/Provisioners/ViewWorkerTypes/index.jsx

+                  provisionerId={provisionerId}
+                  onPageChange={this.handlePageChange}
+                  awsProvisionerWorkerTypeSummaries={
+                    awsProvisionerWorkerTypeSummaries


lol prettier

eliperelman · 2018-04-24T14:46:10Z

src/views/Provisioners/index.jsx

+    return (
+      <Switch>
+        <RouteWithProps
+          path={`${path}/:provisionerId/worker-types/:workerType/workers/:workerGroup?/:workerId?`}


I see workerGroup and workerId are optional, but I couldn't find how their presence affected a view. Could you point me to the right place?

This needs to be removed. We don't have dropdowns in the worker view anymore since most users come to this page via the workers view. I will revise.

helfi92 · 2018-04-26T19:13:27Z

Provisioners

Worker Types

Workers

Worker

eliperelman · 2018-04-26T19:56:44Z

src/components/DateDistance/index.jsx

-    const fromNow = distanceInWordsToNow(from, { addSuffix: true });
-    const offsetNow = offset && distanceInWords(offset, from);
+    const fromNow = formatDistanceStrict(from, new Date(), { addSuffix: true });
+    const offsetNow = offset && formatDistance(from, offset);


Do these new methods return the same output as before? I noticed the version bump, but not sure what the difference is.

Yeah, they were renamed and the arguments order was switched. The change is well explained in the CHANGELOG.

eliperelman · 2018-04-26T19:57:56Z

src/components/DialogAction/index.jsx

+/**
+ * A button that displays a dialog when clicked.
+ */
+export default class DialogAction extends PureComponent {


This needs to be a Component since the props can't be shallowly compared.

eliperelman · 2018-04-26T19:59:47Z

src/components/Dropdown/index.jsx

+/**
+ * A dropdown menu.
+ */
+export default class Dropdown extends Component {


I'm still not sure the point of this component.

eliperelman · 2018-04-26T20:03:31Z

src/components/WorkerTable/index.jsx

+  getTableData = memoizeWith(
+    ({ sortBy, sortDirection }) => `${sortBy}-${sortDirection}`,
+    () => {
+      const { worker } = this.props;


I think these should still be arguments to this function. Pass the props and state you need to the memoizer so it stays pure.

eliperelman · 2018-04-26T20:05:16Z

src/components/WorkerTable/index.jsx

+  );
+
+  handleHeaderClick = sortByHeader => {
+    const sortBy = sortByHeader;


This variable doesn't do anything, so you can rename the argument to this name.

eliperelman · 2018-04-26T20:10:48Z

src/components/WorkerTypesTable/index.jsx

+                <Button
+                  className={classes.infoButton}
+                  size="small"
+                  onClick={() => this.handleDrawerOpen(workerType)}>


Let's use the generated ID approach here too.

@eliperelman workerType is an object here, not a string. How would you gain reference to the object without having to loop over the sorted worker types connection?

You would need to loop over the sorted connection to look up the worker type by the ID.

Wouldn't this make it O(n)? The current implementation is O(1).

It also creates n event handlers instead of 1.

eliperelman · 2018-04-26T20:12:12Z

src/components/WorkersTable/index.jsx

+
+  createSortedWorkersConnection = memoizeWith(
+    (workersConnections, sortBy, sortDirection) => {
+      const sorted = pipe(


Move definition outside class so it's not regenerated every call.

eliperelman · 2018-04-26T20:13:17Z

src/components/WorkersTable/index.jsx

+      return `${ids.join('-')}-${sortBy}-${sortDirection}`;
+    },
+    () => {
+      const { workersConnection } = this.props;


Use the props and state from arguments to keep this pure.

eliperelman · 2018-04-26T20:16:09Z

src/components/WorkersTable/index.jsx

+            </TableCell>
+            <TableCell>
+              {quarantineUntil
+                ? formatDistanceStrict(Date(), quarantineUntil, { unit: 'd' })


new Date()?

The parameter can be of type Date | String | Number https://date-fns.org/v2.0.0-alpha.7/docs/formatDistanceStrict#arguments. I'll be sure to be consistent everywhere.

eliperelman · 2018-04-26T20:20:38Z

src/views/Provisioners/ViewWorkers/index.jsx

+                  workerType={params.workerType}
+                  provisionerId={params.provisionerId}
+                />
+                <SpeedDial


We can remove the speed dial if there are no actions.

eliperelman · 2018-05-01T11:47:20Z

src/components/DateDistance/index.jsx

@@ -26,7 +26,7 @@ export default class DateDistance extends Component {

  render() {
    const { from, offset } = this.props;
-    const fromNow = formatDistanceStrict(from, new Date(), { addSuffix: true });
+    const fromNow = formatDistanceStrict(from, Date(), { addSuffix: true });


Change Date to new Date().

eliperelman · 2018-05-01T11:51:31Z

src/views/Provisioners/ViewWorkers/index.jsx

+                        tooltipTitle={
+                          <div>
+                            <div>{action.title}</div>
+                            <div>{action.description}</div>


Remove description. The description will be in the dialog that gets opened, and putting it here will be potentially too large and redundant to show every time.

eliperelman · 2018-05-01T11:51:59Z

src/views/Provisioners/ViewWorkers/index.jsx

+                            <div>{action.description}</div>
+                          </div>
+                        }
+                        onClick={() => this.handleActionClick(action)}


We can leave this for now if it keeps it simple. Maybe add a TODO to come back later.

helfi92 added 27 commits April 19, 2018 13:00

Create ButtonDrawer component

e369b5e

Create DialogAction component

e5bbac4

Create Dropdown component

43c2980

Implemented so that we have consistent dropdowns across the site

Create DataTable component

c24897a

A similar implementation to the ConnectionDataTable except that it takes raw data and no pagination

Lint

d95f056

Add more labels mappings in util

b82645a

Add page size constants

01f911b

Modify theme

fb464af

Add a sort utility to help when sorting tables

4c8b2cf

Remove hard-coded font size in Markdown component

4b13244

Add sorting feature to ConnectionDataTable

23cf8fb

Add 'change-case' to package.json and update yarn.lock

3392175

Add new prop-types for provisioner, worker type, and worker

47bec24

Create ProvisionerDetailsCard component

291151d

Create WorkerDetailsCard component

bb9e803

Create WorkerTypeMetadataCard component

237157a

Create WorkerTypeMetadataCard component

463d8a4

Create QuarantineButton component

e930de1

`QuarantineButton` was created mainly because it has somewhat a complex dialog where a user can modify the quarantineUntil date. To isolate the state management as well as possible code duplication, this component was created.

Create TableCellListItem component

0dafa4c

This component integrates gracefully in a TableCell. It aligns list items with column headers along with other styling which otherwise needs to be duplicated in every component that would use a `ListItem` inside a `TableCell`.

Create WorkerTypesTable component

668428f

Create WorkersTable component

f95cd86

Create WorkerTable component

35bf6be

Add the views

9b979d9

Add /provisioners as the parent route

65adee0

Lint

bcf9fad

Put info icon to the left of task name

a8223a2

Done in order to be consistent with the worker types view

Fix paging in the Workers view

5a9f7a5

helfi92 self-assigned this Apr 19, 2018

helfi92 requested a review from eliperelman as a code owner April 19, 2018 18:35

eliperelman suggested changes Apr 21, 2018

View reviewed changes

Add requested changes for round 1 comments

031e8be

helfi92 requested a review from eliperelman April 23, 2018 19:36

eliperelman reviewed Apr 24, 2018

View reviewed changes

eliperelman suggested changes Apr 24, 2018

View reviewed changes

Add requested changes for round 2 comments

9e1f3d9

helfi92 requested a review from eliperelman April 26, 2018 19:14

eliperelman suggested changes Apr 26, 2018

View reviewed changes

Add requested changes for round 3 comments

9456742

helfi92 requested a review from eliperelman April 26, 2018 22:31

eliperelman suggested changes May 1, 2018

View reviewed changes

Add requested changes for round 4 comments

edc11a5

helfi92 requested a review from eliperelman May 1, 2018 13:25

This was referenced May 1, 2018

Create/edit clients #34

Closed

Clients list #33

Closed

eliperelman approved these changes May 2, 2018

View reviewed changes

helfi92 merged commit 70f0e27 into taskcluster:master May 2, 2018

Provisioners, Worker Types, Workers, Worker #65

Provisioners, Worker Types, Workers, Worker #65

Conversation

helfi92 commented Apr 19, 2018

helfi92 commented Apr 19, 2018

View Provisioners

View Worker Types

View Workers

View Worker

eliperelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 Apr 24, 2018 • edited

Choose a reason for hiding this comment

eliperelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 commented Apr 26, 2018

Provisioners

Worker Types

Workers

Worker

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 Apr 24, 2018 •

edited