Skip to content

#7 Permissions #9

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

Merged
merged 10 commits into from
Mar 19, 2018
Merged
Show file tree
Hide file tree
Changes from 6 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
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
"private": true,
"dependencies": {
"emotion": "^9.0.2",
"interweave": "^8.0.2",
"makecancelable": "^1.0.0",
"prop-types": "^15.6.1",
"react": "^16.2.0",
"react-dom": "^16.2.0",
Expand Down
165 changes: 152 additions & 13 deletions src/components/05_pages/Permissions/Permissions.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,156 @@
import React from 'react';
import { css } from 'emotion';
import React, { Component, Fragment } from 'react';
import makeCancelable from 'makecancelable';
import { Markup } from 'interweave';

const styles = {
title: css`
text-decoration: underline;
`,
};
import Loading from '../../Helpers/Loading';
import { Table, TableBody, TableHeaderSimple } from '../../UI';

const Permissions = class Permissions extends Component {
state = {
loaded: false,
};
componentDidMount() {
this.cancelFetch = makeCancelable(
Promise.all([
fetch(
`${
process.env.REACT_APP_DRUPAL_BASE_URL
}/admin-api/permissions?_format=json`,
).then(res => res.json()),
fetch(
`${
process.env.REACT_APP_DRUPAL_BASE_URL
}/jsonapi/user_role/user_role`,
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to add the Accept: application/vnd.api+json header. This will enable 404/403 errors in JSON format (if any). Otherwise they'll come back as HTML errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

).then(res => res.json()),
])
.then(([permissions, { data: roles }]) =>
this.setState({
rawPermissions: Object.keys(permissions).map(
key => permissions[key],
),
renderablePermissions: Object.keys(permissions).map(
key => permissions[key],
),
changedRoles: [],
// Move admin roles to the right.
roles: roles.sort((a, b) => {
if (a.attributes.is_admin && b.attributes.is_admin) {
return a.attributes.id - b.attributes.id;
} else if (a.attributes.is_admin) {
return 1;
} else if (b.attributes.is_admin) {
return -1;
}
return a.attributes.id - b.attributes.id;
}),
loaded: true,
}),
)
.catch(err => this.setState({ loaded: false, err })),
);
}
componentWillUnmount() {
this.cancelFetch();
}
onPermissionCheck = (roleName, permission) => {
this.setState(prevState => ({
changedRoles: [...new Set(prevState.changedRoles).add(roleName).values()],
roles: this.togglePermission(permission, roleName, prevState.roles),
}));
};

togglePermission = (permission, roleName, roles) => {
const roleIndex = roles.map(role => role.attributes.id).indexOf(roleName);
const role = roles[roleIndex];
const index = role.attributes.permissions.indexOf(permission);
if (index !== -1) {
role.attributes.permissions.splice(index, 1);
} else {
role.attributes.permissions.push(permission);
}
roles[roleIndex] = role;
return roles;
};

const Permissions = () => (
<div>
<h1 className={styles.title}>Permissions</h1>
<p>This will be the permissions page.</p>
</div>
);
groupPermissions = permissions =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice code!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing we will need to figure out, is how to handle polyfills. Since this uses entries we will want to figure that out.

Object.entries(
permissions.reduce((acc, cur) => {
acc[cur.provider] = acc[cur.provider] || [];
acc[cur.provider].push(cur);
return acc;
}, {}),
);

createTableRows = (groupedPermissions, roles) =>
[].concat(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why [].concat() instead of array spreads? It seems you prefer spreads elsewhere.

Copy link
Contributor

@e0ipso e0ipso Mar 16, 2018

Choose a reason for hiding this comment

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

For clarity, I'm not after any change here, I'm genuinely curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@e0ipso It's a bit subtle, but this is the easiest way to flatten the two-dimensional array that's created from constructing the table rows.

...groupedPermissions.map(([permissionGroupName, permissions]) => [
{
key: `permissionGroup-${permissionGroupName}`,
colspan: roles.length + 1,
tds: [<b>{permissionGroupName}</b>],
},
...permissions.map(permission => ({
key: `permissionGroup-${permissionGroupName}-${permission.title}`,
tds: [
<Markup content={permission.title} />,
...roles.map(
({ attributes }) =>
attributes.is_admin && attributes.id === 'administrator' ? (
<input type="checkbox" checked />
) : (
<input
type="checkbox"
onChange={() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering: Is there a way to avoid creating new functions for each rendered checkbox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dawehner Let me investigate that.

this.onPermissionCheck(attributes.id, permission.id)
}
checked={attributes.permissions.includes(permission.id)}
/>
),
),
],
})),
]),
);
handleKeyPress = event => {
const input = event.target.value.toLowerCase();
this.setState(prevState => ({
...prevState,
renderablePermissions: prevState.rawPermissions.filter(
({ title, description, provider, provider_label: providerLabel }) =>
`${title}${description}${provider}${providerLabel}`.includes(input),
),
}));
};
render() {
return !this.state.loaded ? (
<Loading />
) : (
<Fragment>
<input
type="text"
placeholder="Filter by name, description or module"
onChange={this.handleKeyPress}
onKeyDown={this.handleKeyPress}
/>
<Table zebra>
<TableHeaderSimple
data={[
'PERMISSION',
...this.state.roles.map(({ attributes: { label } }) =>
label.toUpperCase(),
),
]}
/>
<TableBody
rows={this.createTableRows(
this.groupPermissions(this.state.renderablePermissions),
this.state.roles,
)}
/>
</Table>
</Fragment>
);
}
};

export default Permissions;
37 changes: 37 additions & 0 deletions src/components/Helpers/Loading.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import React from 'react';
import { css } from 'emotion';

const styles = {
wrap: css`
margin: 100px auto 0;
`,
peace: css`
display: inline-block;
vertical-align: middle;
animation-direction: alternate;
animation-iteration-count: infinite;
animation-duration: 0.5s;
animation-timing-function: cubic-bezier(0, 0, 1, 1);
transform-origin: bottom;
font-size: 50px;
animation-name: rotate;
@keyframes rotate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually work? I've done keyframes in emotion according to https://github.com/emotion-js/emotion/blob/master/docs/keyframes.md

const rotate = keyframes`
  //...
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does work.

loading

Copy link
Contributor

Choose a reason for hiding this comment

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

✌️

Copy link

Choose a reason for hiding this comment

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

This PR is amazing

from {
transform: rotate(-10deg);
}
to {
transform: rotate(10deg);
}
}
`,
};

const Loading = () => (
<div className={styles.wrap}>
<span className={styles.peace} role="img" aria-label="Peace Sign">
✌️
Copy link
Contributor

Choose a reason for hiding this comment

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

✌️

</span>
</div>
);

export default Loading;
9 changes: 9 additions & 0 deletions src/components/UI/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { TR, TD, TABLE, TBODY, THEAD } from './table';

export {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do this instead of directly importing from a table file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this so if we ever have any other components, they are all importable from a single file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I think we should come back at some point and have a look at the bigger picture to understand how we can make the entire app more readable and not just single bits and pieces.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a look at some other libraries and the lodash method seems to be popular where you pull in the thing you need like import { td, tr } from 'ui/table', but yeah we can address this in a follow up, doesn't need to block this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justafish @dawehner We can pretty easily switch up the exports here. Probably better to set a good initial pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattgrill cool cool, let's do away with index.js then. Not sure what we should have the exports as exactly? My preference would be:TD, TR, Table, TBody, and THead

TR as TableRow,
TD as TableData,
TABLE as Table,
TBODY as TableBody,
THEAD as TableHeaderSimple,
};
81 changes: 81 additions & 0 deletions src/components/UI/table.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import React from 'react';
import { css } from 'emotion';
import {
node,
bool,
oneOfType,
arrayOf,
string,
shape,
number,
} from 'prop-types';

const generateIDs = arr =>
arr.map(value => ({
value,
id: Math.random()
.toString(36)
.substring(2),
}));

const TABLE = ({ children, zebra, ...props }) => {
const styles = css`
${zebra ? 'tbody tr:nth-child(odd) {background-color: #e8e8e8;}' : ''};
`;
return (
<table className={styles} {...props}>
{children}
</table>
);
};
TABLE.propTypes = {
children: oneOfType([arrayOf(node), node]).isRequired,
zebra: bool,
};
TABLE.defaultProps = {
zebra: false,
};

const TR = ({ children, ...props }) => <tr {...props}>{children}</tr>;
TR.propTypes = {
children: oneOfType([arrayOf(node), node]).isRequired,
};

const TD = ({ children, ...props }) => <td {...props}>{children}</td>;
TD.propTypes = {
children: oneOfType([arrayOf(node), node]).isRequired,
};

const THEAD = ({ data }) => (
<thead>
<TR>{data.map(label => <TD key={`column-${label}`}>{label}</TD>)}</TR>
</thead>
);
THEAD.propTypes = {
data: arrayOf(string).isRequired,
};

const TBODY = ({ rows }) => (
<tbody>
{rows.map(({ colspan, tds, key }) => (
<TR key={key}>
{generateIDs(tds).map(({ id, value }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the advantage of doing this over using the array index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

React gets sad if you use the index as part of the key.

<TD key={`td-${key}-${id}`} colSpan={colspan || undefined}>
{value}
</TD>
))}
</TR>
))}
</tbody>
);
TBODY.propTypes = {
rows: arrayOf(
shape({
colspan: number,
key: string,
tds: arrayOf(node).isRequired,
}),
).isRequired,
};

export { TR, TD, TABLE, TBODY, THEAD };
11 changes: 11 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3700,6 +3700,13 @@ interpret@^1.0.0:
version "1.1.0"
resolved "https://registry.yarnpkg.com/interpret/-/interpret-1.1.0.tgz#7ed1b1410c6a0e0f78cf95d3b8440c63f78b8614"

interweave@^8.0.2:
version "8.0.2"
resolved "https://registry.yarnpkg.com/interweave/-/interweave-8.0.2.tgz#51001199c58d311f8b1d0100c4f6b9d494e97480"
dependencies:
babel-runtime "^6.26.0"
prop-types "^15.6.0"

invariant@^2.2.0, invariant@^2.2.1, invariant@^2.2.2:
version "2.2.3"
resolved "https://registry.yarnpkg.com/invariant/-/invariant-2.2.3.tgz#1a827dfde7dcbd7c323f0ca826be8fa7c5e9d688"
Expand Down Expand Up @@ -4647,6 +4654,10 @@ make-dir@^1.0.0:
dependencies:
pify "^3.0.0"

makecancelable@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/makecancelable/-/makecancelable-1.0.0.tgz#c7e2606e59db7a4bf8098ff5b52d7f13173499e5"

[email protected]:
version "1.0.11"
resolved "https://registry.yarnpkg.com/makeerror/-/makeerror-1.0.11.tgz#e01a5c9109f2af79660e4e8b9587790184f5a96c"
Expand Down