Feature/auth0 login #3

katherinebernstein · 2016-09-21T01:19:02Z

react framework set up with routing and login/logout

* develop:

UnsungHero97 · 2016-09-21T01:41:37Z

.babelrc

@@ -1,6 +1,7 @@
 {
  "presets": [
    [ "es2015", { "modules": false } ],
+    "stage-0",


stage-0 gives you stages 1-3, plus these:

transform-do-expressions

transform-function-bind

I'm willing to bet you don't actually need stage-0... what syntax are you going for?

UnsungHero97 · 2016-09-21T01:43:00Z

auth0-variables.js

@@ -0,0 +1,3 @@
+var AUTH0_CLIENT_ID='KvwsETaUxuXVjW2cmz2LbdqXQBdYs6wH'; 


no more vars in ES6. use const. also, this file doesn't seem to be used anywhere, and consts.js seems to have the same data.

looks like you don't need this file?

UnsungHero97 · 2016-09-21T01:45:05Z

config/consts.js

@@ -3,14 +3,19 @@
 */

 const APP_INDEX_HTML = 'index.html';
-const APP_INDEX_JS = 'index.js';
+const APP_INDEX_JS = 'app.js';


since you're changing index.js to app.js, why not also change the constant name from APP_INDEX_JS to just APP_JS?

UnsungHero97 · 2016-09-21T01:46:41Z

config/consts.js


 const MEDIA_FILE_REGEX = /\.(ico|jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2)(\?.*)?$/;
 const MEDIA_URL_REGEX = /\.(mp4|webm|wav|mp3|m4a|aac|oga)(\?.*)?$/;

+const AUTH0_CLIENT_ID = 'KvwsETaUxuXVjW2cmz2LbdqXQBdYs6wH';
+const AUTH0_DOMAIN = 'loom.auth0.com';
+
 module.exports = {


module.exports is a NodeJS thing. in ES6, we'll be using the export keyword. look at this for reference:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export
eslint/espree#43

so this will translate to...

export default { ... }

export default { ... } seems to be breaking it: "SyntaxError: Unexpected token export". (also that part came from the lets-react project, I think)

you're right. this has to stay module.exports for now :/

UnsungHero97 · 2016-09-21T01:48:39Z

config/webpack.config.base.js


 const CONSTS = require('./consts');
 const ENV = require('./env.js');
 const PATHS = require('./paths.js');
 const PACKAGE = require('../package.json');
+const root    = resolve(__dirname);


already exists... PATHS.ABS.ROOT. you could do const ROOT = PATHS.ABS.ROOT to make it easier to type

UnsungHero97 · 2016-09-21T01:50:04Z

config/webpack.config.base.js


 const CONSTS = require('./consts');
 const ENV = require('./env.js');
 const PATHS = require('./paths.js');
 const PACKAGE = require('../package.json');
+const root    = resolve(__dirname);
+const src     = join(root, '../src');


already exists... PATHS.ABS.SOURCE

UnsungHero97 · 2016-09-21T01:50:51Z

config/webpack.config.base.js


 const CONSTS = require('./consts');
 const ENV = require('./env.js');
 const PATHS = require('./paths.js');
 const PACKAGE = require('../package.json');
+const root    = resolve(__dirname);
+const src     = join(root, '../src');
+const dest    = join(root, '../dist');


we're not using dist. we're using build. available in PATHS.ABS.BUILD

UnsungHero97 · 2016-09-21T01:51:24Z

config/consts.js


 const MEDIA_FILE_REGEX = /\.(ico|jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2)(\?.*)?$/;
 const MEDIA_URL_REGEX = /\.(mp4|webm|wav|mp3|m4a|aac|oga)(\?.*)?$/;

+const AUTH0_CLIENT_ID = 'KvwsETaUxuXVjW2cmz2LbdqXQBdYs6wH';


is this meant to be a secret token, or open to the public?

it's the public token!

UnsungHero97 · 2016-09-21T01:51:56Z

config/webpack.config.base.js

+const path    = require('path'),
+      join    = path.join,
+      resolve = path.resolve;
+const getConfig = require('hjs-webpack');


why do you need hjs-webpack? also, you're not using getConfig anywhere.

UnsungHero97 · 2016-09-21T01:56:25Z

package.json

@@ -40,12 +46,29 @@
    "eslint-plugin-react": "^6.0.0",
    "file-loader": "^0.9.0",
    "flow-bin": "^0.32.0",
+    "hjs-webpack": "^8.1.0",


you don't need to worry about webpack configuration and such. I'll take care of all of that. also, hjs-webpack is basically what our lets-react is meant to be. why are you including it?

UnsungHero97 · 2016-09-21T01:57:22Z

package.json

    "babel-preset-es2015": "^6.14.0",
    "babel-preset-react": "^6.11.1",
+    "babel-preset-stage-0": "^6.5.0",
+    "bootstrap": "^3.3.5",


looks like you're using react-bootstrap everywhere, which is good. so you don't need this dependency.

UnsungHero97 · 2016-09-21T02:01:09Z

src/app.css

@@ -0,0 +1,22 @@
+@import url("styles/colors.css");


please be consistent with your whitespace. I'll assign myself a task for linting CSS

what did you want me to change about the whitespace here?

you're using 4 space indentation and 2 space indentation. should be 2space across the board

oh haha whoops

UnsungHero97 · 2016-09-21T02:05:14Z

src/views/Main/routes.js

+import Catalog from './Catalog/Catalog'
+import Login from './Login/Login'
+
+const CONSTS = require('../../../config/consts');


don't use require. use the import syntax.

refer to my above comments. ../../../config/consts is the wrong location for these Auth0 constants. they should live under src

UnsungHero97 · 2016-09-21T02:07:29Z

config/consts.js


 const MEDIA_FILE_REGEX = /\.(ico|jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2)(\?.*)?$/;
 const MEDIA_URL_REGEX = /\.(mp4|webm|wav|mp3|m4a|aac|oga)(\?.*)?$/;

+const AUTH0_CLIENT_ID = 'KvwsETaUxuXVjW2cmz2LbdqXQBdYs6wH';
+const AUTH0_DOMAIN = 'loom.auth0.com';
+
 module.exports = {


the top level config folder is meant to be for general project configuration, for tasks like building, linting, deploying, etc...

this is application specific, so it belongs under src. you can either create a config folder under src, or something else entirely. up to you.

I'll standardize all of this in lets-react at some point, and provide better documentation to provide more clarity.

UnsungHero97

just a heads up... you'll have a lot of errors if you run ESLint on this code. linting is not fully set up for you yet, sorry, but it's not hard to run ESLint against this

UnsungHero97 · 2016-09-21T02:09:55Z

src/views/Main/routes.js

+  return (
+    <Route path="/" component={Container} auth={auth}>
+      <IndexRedirect to="/catalog" />
+      <Route path="catalog" component={Catalog} onEnter={requireAuth} />


path="catalog" probably works fine, but I think it's good to be consistent. so I'd change this to path={'catalog'}.

UnsungHero97 · 2016-09-21T02:10:42Z

src/containers/App/App.spec.js

+import { shallow } from 'enzyme'
+
+import App from './App'
+import styles from './styles.module.css'


this is not used anywhere

UnsungHero97 · 2016-09-22T17:38:46Z

package.json

@@ -20,20 +20,23 @@
  "author": "Hristo Oskov",
  "license": "Apache-2.0",
  "dependencies": {
+    "auth0-lock": "^10.3.0",


dependencies are required for the app to run. devDependencies are required only for development. anything related to unit tests should be under devDependencies. I believe anything that's being bundled will be under dependencies.

react

react-dom

react-router (I think)

react-bootstrap (I think)

auth0-lock

ok! I moved chai out, all the others are needed otherwise the linter gets sad

UnsungHero97 · 2016-09-22T17:40:05Z

src/app.js


-import 'bootstrap/dist/css/bootstrap.css'
-import './app.css'
+import 'react-bootstrap/dist/react-bootstrap.js';


this looks like invalid syntax.

you're not using bootstrap in this file, so you don't need the import.

UnsungHero97 · 2016-09-22T17:40:54Z

src/app.js

-import './app.css'
+import 'react-bootstrap/dist/react-bootstrap.js';
+import { hashHistory } from 'react-router';
+import './app.css';


check syntax. also doesn't look like it's being used.

UnsungHero97 · 2016-09-22T17:45:05Z

src/containers/App/App.js


 class App extends React.Component {
  static contextTypes = {
    router: PropTypes.object
  }

  static propTypes = {
-    history: PropTypes.object.isRequired,
    routes: PropTypes.element.isRequired
  };

  get content() {


these getters are usually used for binding a function to an object property. content is more than just a property in this case, so I think it's better to make it a class method.

since you're returning JSX, how about renderContent?

UnsungHero97 · 2016-09-22T17:47:07Z

src/utils/AppConsts.js

+const AUTH0_CLIENT_ID = 'KvwsETaUxuXVjW2cmz2LbdqXQBdYs6wH';
+const AUTH0_DOMAIN = 'loom.auth0.com';
+
+module.exports = {


module.exports is for NodeJS. you can use standard ES6 export syntax for any of the app source code.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export

UnsungHero97 · 2016-09-22T17:48:23Z

src/views/Main/Catalog/Catalog.js

-import styles from './styles.module.css'
+import React, { PropTypes as T } from 'react';
+import { Button } from 'react-bootstrap';
+import AuthService from '../../../utils/AuthService';


I'll figure out a way to make this nicer :)

cool yay thanks!

UnsungHero97 · 2016-09-22T17:49:53Z

src/views/Main/Catalog/Catalog.js

-import {Button} from 'react-bootstrap'
-import AuthService from 'utils/AuthService'
-import styles from './styles.module.css'
+import React, { PropTypes as T } from 'react';


what is this for { PropTypes as T }? doesn't look like PropTypes is being used, nor T

it's being used for auth things. I got rid of the alias though

UnsungHero97 · 2016-09-22T17:50:53Z

src/views/Main/Container.js

-import React, { PropTypes as T } from 'react'
-import { Jumbotron } from 'react-bootstrap'
-import styles from './styles.module.css'
+import React, { PropTypes as T } from 'react';


I wouldn't alias PropTypes as T.

UnsungHero97 · 2016-09-22T18:00:09Z

src/views/Main/routes.js

-    </Route>
-  )
-}
+export const makeMainRoutes = () => (


there's 2 files named routes.js, and they both seem to do similar things. is this intentional?

yeah, one is a wrapper around the other. this is the way it came in the default auth0 project, but I can rename one of them to make it less confusing, or see if I can combine them somehow, if you think that would help.

Release #3 3-13-2017

Release #3 4/20

Katherine added 3 commits September 20, 2016 18:06

login/logout functionality works!!!

9b4407f

Merge branch 'develop' into feature/auth0-login

1342166

* develop:

rename home to catalog

dcc3df0

katherinebernstein assigned UnsungHero97 Sep 21, 2016

UnsungHero97 reviewed Sep 21, 2016

View reviewed changes

Katherine added 5 commits September 21, 2016 10:56

cleanup/hristo's changes

cf74710

initial linter fixes

4d4ff0c

more fixes

a9d6ef9

more lint errors fixed, don't forbid object as property type

276bd6d

the linter has been appeased

b5ceb84

UnsungHero97 reviewed Sep 22, 2016

View reviewed changes

Katherine added 2 commits September 22, 2016 13:00

more cleanup

675b3ed

PropTypes -> T

d602e5c

UnsungHero97 approved these changes Sep 28, 2016

View reviewed changes

katherinebernstein merged commit 5c36faf into develop Sep 28, 2016

katherinebernstein deleted the feature/auth0-login branch October 2, 2016 22:01

geekbeast added a commit that referenced this pull request Mar 13, 2017

Merge pull request #276 from kryptnostic/develop

24c5e5f

Release #3 3-13-2017

corwinc mentioned this pull request Mar 21, 2017

Feature/view all permissions #292

Merged

geekbeast added a commit that referenced this pull request May 2, 2017

Merge pull request #328 from kryptnostic/develop

6ddc879

Release #3 4/20

		@@ -0,0 +1,3 @@
		var AUTH0_CLIENT_ID='KvwsETaUxuXVjW2cmz2LbdqXQBdYs6wH';

Feature/auth0 login #3

Feature/auth0 login #3

Conversation

katherinebernstein commented Sep 21, 2016

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

UnsungHero97 Sep 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UnsungHero97 Sep 21, 2016 • 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

UnsungHero97 Sep 21, 2016 • edited

Choose a reason for hiding this comment

UnsungHero97 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

UnsungHero97 Sep 21, 2016 •

edited

UnsungHero97 Sep 21, 2016 •

edited

UnsungHero97 Sep 21, 2016 •

edited