#7 Permissions #9

mattgrill · 2018-03-06T23:00:37Z

Setup

Enable JSON API and Admin UI supportmodules.

Permissions

Request and render permissions list from Drupal.
Need to refine json from drupal for permissions list, why are we sending html?
Need to figure out how to reference current permission to user role data.
Live filtering?
Styles

dawehner · 2018-03-06T23:19:50Z

src/components/05_pages/Permissions/Permissions.jsx

+        });
+        return { groupedPermissions, loaded: true };
+      })
+      .then(data => this.setState({ ...data }))


Why can we not use .then(data => this.setState)?

dawehner · 2018-03-06T23:20:02Z

src/components/05_pages/Permissions/Permissions.jsx

+        return { groupedPermissions, loaded: true };
+      })
+      .then(data => this.setState({ ...data }))
+      .catch(console.log);


Let's console.error that?

Maybe we could route it through winston and have logger.error here instead?
https://www.npmjs.com/package/winston

Should we create a quick ticket to track this? For me ideally we write the data one day back to Drupal

dawehner · 2018-03-06T23:21:50Z

.eslintrc.json

+{
+  "extends": [
+    "eslint-config-airbnb",
+    "plugin:prettier/recommended"


dawehner · 2018-03-06T23:24:57Z

src/components/05_pages/Permissions/Permissions.jsx

+                  <Fragment key={`fragment-${permissionGroupName}`}>
+                    <tr key={`permissionGroup-${permissionGroupName}`}>
+                      <td>{permissionGroupName}</td>
+                    </tr>


I just love how prettier ensures things aren't too dense, honestly!

dawehner · 2018-03-06T23:25:08Z

Need to refine json from drupal for permissions list, why are we sending html?

Is this still a problem?

dawehner · 2018-03-06T23:26:40Z

Need to figure out how to reference current permission to user role data.

When you access the user roles they will each have a list of permissions they have

Sortable?

Do you think this is super useful? I can't really think of a usecase for that.

Live filtering?

Yes! See my comment in the PR

dawehner · 2018-03-06T23:27:38Z

src/components/05_pages/Permissions/Permissions.jsx

+    loaded: false,
+  };
+  componentDidMount() {
+    fetch(`${drupalOrigin}/permissions?_format=json`)


I think we should group the permissions when we render. Otherwise live filtering permissions would be a bit more pain. Thoughts?

justafish · 2018-03-07T11:52:01Z

.eslintignore

@@ -0,0 +1 @@
+build/**/*.js


did https://github.com/jsdrupal/drupal-admin-ui/blob/master/.gitignore#L10 not cover this?

justafish · 2018-03-07T11:52:12Z

.eslintrc.json

+{
+  "extends": [
+    "eslint-config-airbnb",
+    "plugin:prettier/recommended"


justafish · 2018-03-07T11:53:17Z

.eslintrc.json

+  "rules": {
+    "no-param-reassign": [0],
+    "no-underscore-dangle": [0],
+    "react/jsx-filename-extension": [1, { "extensions": [".js", ".jsx"] }]


what's the advantage of doing this?

You can put JSX in .js files.

Should we stick with one way? At work we use JS, because well, that's what is in there :)

I'm fine with that, but I think all this stuff needs moving over to a separate issue to discuss how/where we deviate from CRA

justafish · 2018-03-07T11:54:09Z

.prettierrc.json

+  "printWidth": 80,
+  "semi": true,
+  "singleQuote": true,
+  "trailingComma": "all"


justafish · 2018-03-07T11:54:42Z

package.json

@@ -9,9 +9,19 @@
    "react-router-dom": "^4.2.2"
  },
  "devDependencies": {
+    "eslint": "^4.9.0",


Aren't these already included with CRA?

Eslint is included with CRA, but none of the other deps we need for airbnb or prettier plugins.

CRA uses eslint:recommended instead (see facebook/create-react-app#3540 (comment)), but I think we can avoid this when we bump to react-scripts 2.0 (if we do want airbnb here).
Can we split this eslint/prettier stuff into a separate PR so we can continue there and not hold things up here.

justafish · 2018-03-07T11:55:20Z

src/App.jsx

@@ -20,7 +16,7 @@ class NoMatch extends Component {
  render() {
    return null;
  }
-};


Was this a prettier fix?

justafish · 2018-03-07T11:56:06Z

src/components/05_pages/Permissions/Permissions.jsx

-    <p>This will be the permissions page.</p>
-  </div>
-);
+const drupalOrigin = 'http://localhost:9998';


let's add this in with dotenv (already included in CRA)

There is a bit of a pane involved with CRE here. Environment variables aren't passed along unless they are prefixed with react. I had this problem when working on my config experiment, see https://github.com/jsdrupal/drupal-admin-ui/compare/config-experiment-2#diff-01454d5c4c96a618ac33dd690eb3c20bR128

I hope we can workaround that though :)

oh yeah, I forgot about that. I think prefixing with REACT_ is totally fine 👍

justafish · 2018-03-07T11:58:39Z

src/components/05_pages/Permissions/Permissions.jsx

+        return { groupedPermissions, loaded: true };
+      })
+      .then(data => this.setState({ ...data }))
+      .catch(console.log);


Maybe we could route it through winston and have logger.error here instead?
https://www.npmjs.com/package/winston

justafish · 2018-03-07T11:59:40Z

src/components/05_pages/Permissions/Permissions.jsx

+    return (
+      <Fragment>
+        {!this.state.loaded ? (
+          <p>loading...</p>


s/.../… 🤓

I'm wondering whether there is a better thing to show

a party parrot, obviously

dawehner · 2018-03-12T09:30:29Z

src/components/05_pages/Permissions/Permissions.js

+  render() {
+    return (
+      <Fragment>
+        {!this.state.loaded ? (


Here is a generic question. Is there a reason why you do the condition inside the fragment and not outside of it?

Since we're just returning the <table /> here we probably don't even need the fragment at all.

dawehner · 2018-03-12T09:31:12Z

src/components/05_pages/Permissions/Permissions.js

+    loaded: false,
+  };
+  componentDidMount() {
+    Promise.all([


Also a general question. Don't we need some handling for the case that the component is not longer active but the promise resolves? (ideally we would cancel the promise as part of componentDidUnmount or something like that.

Yes, we could set the promise up in the constructor and just execute when the component mounts, and cancel it later.

Let's keep that up as a todo, I guess?

I'll just get these wrapped up today. :)

dawehner · 2018-03-12T23:56:47Z

src/components/05_pages/Permissions/Permissions.js

+  componentWillUnmount() {
+    this.cancelFetch();
+  }
+  fetchData = () =>


I think we can drop this now :)

dawehner · 2018-03-13T00:14:38Z

src/components/05_pages/Permissions/Permissions.js

+        `${process.env.REACT_APP_DRUPAL_BASE_URL}/jsonapi/user_role/user_role`,
+      ).then(res => res.json()),
+    ]);
+  groupPermissions = permissions =>


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

dawehner · 2018-03-13T00:15:02Z

src/components/05_pages/Permissions/Permissions.js

+            ([permissionGroupName, permissions]) =>
+              permissions.length && (
+                <Fragment key={`fragment-${permissionGroupName}`}>
+                  <tr key={`permissionGroup-${permissionGroupName}`}>


Do we need key on both levels?

Yep. Which is strange, but each element within the loop needs a key.

dawehner · 2018-03-13T00:16:10Z

src/components/05_pages/Permissions/Permissions.js

+                            permission.id
+                          }`}
+                        >
+                          {attributes.is_admin &&


Nice level of detail!

dawehner · 2018-03-13T20:29:23Z

src/components/UI/table.js

+  zebra: false,
+};
+
+const TBODY = ({ children, ...props }) => <tbody {...props}>{children}</tbody>;


Oh default HTML elements don't offer proptypes?

So I wanted to provide these wrappers so if we want to provide styling/functionality for basic DOM elements, we can do it all in one place and those changes will be reflected everywhere.

dawehner · 2018-03-13T23:13:42Z

src/components/05_pages/Permissions/Permissions.js

+                ],
+              })),
+            ])
+            .reduce((a, b) => a.concat(b), [])}


Note: You can use [].concat(...a) instead of the reduce.

justafish · 2018-03-15T15:58:38Z

src/components/Helpers/Loading.js

+
+const Loading = () => (
+  <div className={spinner}>
+    <div className="bounce1" />


I've had some really Bad Times(tm) with using class names this way when the project starts to grow. The best way we've found is to have an object called styles and then do className={styles.spinner}, className={styles.bounce1} etc

Yep, this totally makes sense.

I think we resolved this?

dawehner · 2018-03-15T16:49:03Z

src/components/05_pages/Permissions/Permissions.js

@@ -108,7 +100,7 @@ const Permissions = class Permissions extends Component {
                ) : (
                  <input
                    type="checkbox"
-                    onChange={e =>
+                    onChange={() =>


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

@dawehner Let me investigate that.

Co-authored-by: Michael Herchel <mike@lullabot.com>

mherchel · 2018-03-15T18:28:27Z

src/components/Helpers/Loading.js

  peace: css`
-    ::before {
-      content: '✌️';


mherchel · 2018-03-15T18:28:54Z

src/components/Helpers/Loading.js

-    <span className={styles.peace} />
-    <span className={styles.right} />
+    <span className={styles.peace} role="img" aria-label="Peace Sign">
+      ✌️


mherchel · 2018-03-15T18:32:45Z

src/components/Helpers/Loading.js

+    transform-origin: bottom;
+    font-size: 50px;
+    animation-name: rotate;
+    @keyframes rotate {


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` //... `

Yes, it does work.

This PR is amazing

justafish · 2018-03-15T20:35:13Z

src/components/UI/table.js

+  <tbody>
+    {rows.map(({ colspan, tds, key }) => (
+      <TR key={key}>
+        {generateIDs(tds).map(({ id, value }) => (


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

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

justafish · 2018-03-15T20:36:24Z

src/components/UI/index.js

@@ -0,0 +1,9 @@
+import { TR, TD, TABLE, TBODY, THEAD } from './table';
+
+export {


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

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

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.

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

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

@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

e0ipso · 2018-03-16T14:46:00Z

src/components/05_pages/Permissions/Permissions.js

+        fetch(
+          `${
+            process.env.REACT_APP_DRUPAL_BASE_URL
+          }/jsonapi/user_role/user_role`,


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.

e0ipso · 2018-03-16T14:49:56Z

src/components/05_pages/Permissions/Permissions.js

+    );
+
+  createTableRows = (groupedPermissions, roles) =>
+    [].concat(


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

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

@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.

e0ipso · 2018-03-16T14:51:38Z

Pretty awesome. I almost followed most of it.

Keyframes adjustments. Accept header for JSON API requests. Performance improvements for Table Body components.

mattgrill · 2018-03-19T18:09:35Z

Based on the conversation, perhaps we can merge this today?

mattgrill · 2018-03-19T18:15:12Z

mattgrill · 2018-03-19T18:15:44Z

A small highlight of the current permissions page work.

Evaluator improvements

dawehner reviewed Mar 6, 2018

View reviewed changes

justafish requested changes Mar 7, 2018

View reviewed changes

mattgrill force-pushed the permissions branch from 2c5a64b to 16b82c0 Compare March 9, 2018 20:04

dawehner reviewed Mar 12, 2018

View reviewed changes

dawehner reviewed Mar 13, 2018

View reviewed changes

justafish changed the title ~~Permissions~~ #7 Permissions Mar 13, 2018

dawehner reviewed Mar 13, 2018

View reviewed changes

Permissions

05b51be

mattgrill force-pushed the permissions branch from 47d14e3 to 05b51be Compare March 15, 2018 15:36

Parse incoming HTML strings for permission titles with Interweave.

70b19e4

justafish requested changes Mar 15, 2018

View reviewed changes

dawehner and others added 3 commits March 15, 2018 16:28

Store the permissions of each role

7351d07

Move the admin role to the right

7663ba1

Cleanup Permissions. Peace-Wave loading initial work.

ef5701e

dawehner reviewed Mar 15, 2018

View reviewed changes

Updated Peace-Wave loading component.

9cbe922

Co-authored-by: Michael Herchel <mike@lullabot.com>

mherchel reviewed Mar 15, 2018

View reviewed changes

src/components/Helpers/Loading.js Outdated

peace: css`

::before {

content: '✌️';

Copy link

Contributor

mherchel Mar 15, 2018

Choose a reason for hiding this comment

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

✌️

mherchel reviewed Mar 15, 2018

View reviewed changes

justafish reviewed Mar 15, 2018

View reviewed changes

e0ipso reviewed Mar 16, 2018

View reviewed changes

Matthew Grill and others added 4 commits March 16, 2018 09:09

Fixes -

406d2ab

Keyframes adjustments. Accept header for JSON API requests. Performance improvements for Table Body components.

Fixes initial state setup. Adjusts naming of Table exports.

e6126cb

Use human readable provider_label for permissions grouping label

2513f1e

Fix filesystem case issue.

107e1fa

dawehner approved these changes Mar 19, 2018

View reviewed changes

justafish approved these changes Mar 19, 2018

View reviewed changes

justafish merged commit 259b777 into master Mar 19, 2018

dawehner deleted the permissions branch March 19, 2018 18:14

VD39 pushed a commit to VD39/drupal-admin-ui that referenced this pull request Jul 5, 2018

Merge pull request jsdrupal#9 from balsama/eval-experience

d90c99a

Evaluator improvements

		@@ -0,0 +1,9 @@
		import { TR, TD, TABLE, TBODY, THEAD } from './table';

		export {

		@@ -0,0 +1 @@
		build/*/.js

#7 Permissions #9

#7 Permissions #9

Conversation

mattgrill commented Mar 6, 2018 • edited

Setup

Permissions

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

dawehner commented Mar 6, 2018 • edited

dawehner commented Mar 6, 2018 • edited

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

dawehner Mar 7, 2018 • edited

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

dawehner Mar 13, 2018 • edited

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

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

e0ipso Mar 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e0ipso commented Mar 16, 2018

mattgrill commented Mar 6, 2018 •

edited

dawehner commented Mar 6, 2018 •

edited

dawehner commented Mar 6, 2018 •

edited

dawehner Mar 7, 2018 •

edited

dawehner Mar 13, 2018 •

edited

e0ipso Mar 16, 2018 •

edited