Feature/3 ali login page #11

esbb48 · 2016-12-28T17:42:48Z

Add Login Form
Add redux-devtools
Finish Basic Login Flow （暱稱登入，其實類似匿名登入，不會特別開一筆 user 資料存入）

This change is

sharils · 2016-12-31T17:27:37Z

package.json

    "react-router-redux": "^4.0.5",
    "redux": "^3.5.2",
    "redux-action": "^1.0.0",
+    "redux-actions": "^1.2.0",


Use both redux-action and redux-actions?

only redux-actions

sharils · 2016-12-31T17:28:44Z

package.json

    "redux": "^3.5.2",
    "redux-action": "^1.0.0",
+    "redux-actions": "^1.2.0",
+    "redux-devtools-extension": "^1.0.0",


Is it possible to move to redux-devtools-extension to devDependencies?

I think it not ...

sharils · 2016-12-31T17:29:21Z

package.json

    "redux-saga": "^0.13.0"
  },
  "devDependencies": {
+    "ava": "^0.17.0",


This test runner is interesting. How did you know it?

hmm I saw it on Evan's repo

sharils · 2016-12-31T17:36:50Z

src/Auth/LoginContainer.js

+    this.onFormLogin = this.onFormLogin.bind(this);
+  }
+
+  onChangeUsername(event) {


onChangeUsername({ target: { value: username } = {}} = {})?

Hmm, it seems to be a good idea to introduce redux-form to get rid of handling controlled elements by ourself. Thoughts?

onChangeUsername? onUsernameChange?

okay,

redux-form, I survey it. If I want to use redux-form and grommet, I should customize the component.

onUsernameChange, because another one is onFormLogin, it's not onLoginForm

onUsernameChange, because another one is onFormLogin, it's not onLoginForm

Why do you mention this?

sharils · 2016-12-31T17:45:11Z

src/Auth/LoginContainer.js

+
+  onFormLogin() {
+    const { username } = this.props;
+    if (username.length === 0) return;


if (this.props.username.length === 0)?

sharils · 2016-12-31T18:31:05Z

test/Auth/authModule.js

+    type: 'CHANGE_USERNAME',
+    payload: { username },
+  },
+));


You need another test to prove that changeUsername is not returning a hard-coded string that happens to be Ali.

okay, add the other test for bill

sharils · 2016-12-31T18:31:58Z

test/Auth/authModule.js

+    type: 'LOGIN_REQUEST',
+    payload: {},
+  },
+));


You need another test to prove that it always returns an empty object as the payload.

hmm 不懂意思 0 _ 0

或者該說，這是這樣的單元測試能夠呈現出來的嗎 ="=？

你後面提的需求

確保每次都回傳空的 object（意思是要傳值進去，出來的結果還是 object嗎？）

username 是不是串接而成（感覺這要兩次測試確保是不是兩次結果的 contact）

sharils · 2016-12-31T18:33:22Z

test/Auth/authModule.js

+  { username: '' },
+  changeUsername({ username }),
+  { username },
+));


You need another test to prove that its not doing string concat of username

sharils · 2016-12-31T18:34:37Z

test/Auth/authSaga.js

+    gen.next().value,
+    select(),
+  );
+});


You need another test to prove that it does get authModule from the store and use authModule.username to update localStorage

想一下...

sharils · 2016-12-31T18:36:13Z

test/Auth/authSaga.js

+import { LOGIN_REQUEST } from '../../src/Auth/authModule';
+import loginFlow from '../../src/Auth/authSaga';
+
+test('loginFlow', (t) => {


t is not descriptive at all. Is it a convention to use t when you use ava?

hmm . just follow the document sample @@

ok. It is a convention to use ava then.

sharils · 2017-01-01T15:13:32Z

Also this PR is super complex. I suggest do one thing in one PR.

sharils · 2017-01-09T14:32:20Z

src/Auth/LoginContainer.js


-  onChangeUsername(event) {
-    const username = event.target.value.trim();
+  onUsernameChange({ target: { value: username } = {} } = {}) {


No more .trim()?

sharils · 2017-01-09T14:38:56Z

.babelrc

@@ -1,5 +1,5 @@
 {
-  "presets": ["react", "es2015"],
+  "presets": ["react", "es2015", "stage-2"],


How about "presets": ["react", "es2015", "transform-object-rest-spread"], for now?

Whatever way it is, package.json should be updated right?

sharils · 2017-01-09T14:41:34Z

src/store/localStorage.js

 import localStoragePackage from 'localStorage';

-let localStorage = localStoragePackage; // eslint-disable-line
+// eslint-disable-next-line import/no-mutable-exports


In stead of disabling eslint, do you think it's better to just do:

const localStorage = process.env.NODE_ENV === 'test' ? localStoragePackage ? global.window.localStorage;

?

esbb48 · 2017-01-22T17:48:57Z

NOT YET

find a middleware to sync the redux store and localStorage
How about create a class to encapsulate the logic of authentication?
How about get the initialState from localStorage on store create?
About Test
- test/Auth/authModule.js
  - You need another test to prove that it always returns an empty object as the payload.
  - You need another test to prove that its not doing string concat of username
- test/Auth/authSaga.js
  - You need another test to prove that it does get authModule from the store and use authModule.username to update localStorage

sharils · 2017-01-22T17:56:52Z

src/Root/routes.js

  } else if (!isAuthenticated && pathname !== '/login') {
    replace('/login');
+  } else if (isAuthenticated) {
+    return;


I just realise why this looks strange to me. Basically, /login is a public page. However, you nest /login beneath private page root. Thus you are working around your own auth check code.

sharils · 2017-01-22T18:01:04Z

test/Auth/authModule.js

+const theOtherUsername = 'Bill';

-test('Action: changeUsername', actionTest(
+test('Action: changeUsername: Alice', actionTest(


This test description doesn't really help in terms of understanding what's the purpose of this test. Same as the next test.

sharils · 2017-01-22T18:03:14Z

argh.... It seems that I review your work in progress ...........

sharils · 2017-01-22T18:03:33Z

I guess we need a protocol. how about when you are done, ping me on fb?

sharils · 2017-01-22T18:04:53Z

Do you want to give https://reviewable.io/ a try? I can set it up.

esbb48 · 2017-02-07T17:44:12Z

Review status: 0 of 20 files reviewed at latest revision, 37 unresolved discussions.

src/Auth/LoginContainer.js, line 20 at r1 (raw file):

Previously, sharils (sharils) wrote…

onUsernameChange, because another one is onFormLogin, it's not onLoginForm

Why do you mention this?

I just want to tell you that I use onUsernameChange .

Because it's same rule with onFormLogin.

src/Auth/LoginContainer.js, line 39 at r1 (raw file):

Previously, sharils (sharils) wrote…

Because I have not done it yet, I just put the button for the true position.

From this, you should get a feedback that the scope of your issue is too big.

okay

src/Auth/LoginContainer.js, line 48 at r1 (raw file):

Previously, sharils (sharils) wrote…

required is a standard HTML5 attribute. If you can't use it with grommet. There is a bigger problem... https://www.w3.org/TR/html5/forms.html#the-required-attribute

I think the pull request is too big.

If I real want to change the template, I will open the other issue to resolve this problem.

src/Auth/LoginContainer.js, line 50 at r1 (raw file):

Previously, sharils (sharils) wrote…

OK, it seems that footer can apply to section. http://stackoverflow.com/questions/11067186/is-using-footer-inside-form-semantically-fine

got it , 3Q!

src/Auth/LoginContainer.js, line 20 at r3 (raw file):

Previously, sharils (sharils) wrote…

ping?

why you ping this?
It's already done. @@

src/store/localStorage.js, line 3 at r5 (raw file):

Previously, sharils (sharils) wrote…

ping?

done

test/Auth/authModule.js, line 10 at r1 (raw file):

Previously, sharils (sharils) wrote…

This is actually related to don't be cute with your test data in 97things....

uh oh ...

test/Auth/authSaga.js, line 6 at r1 (raw file):

Previously, sharils (sharils) wrote…

ok. It is a convention to use ava then.

hmm

Comments from Reviewable

esbb48 · 2017-02-07T17:45:17Z

ok

Review status: 0 of 20 files reviewed at latest revision, 37 unresolved discussions.

Comments from Reviewable

esbb48 · 2017-02-08T16:55:14Z

Review status: 0 of 20 files reviewed at latest revision, 37 unresolved discussions.

src/Root/routes.js, line 16 at r8 (raw file):

Previously, sharils (sharils) wrote…

I just realise why this looks strange to me. Basically, /login is a public page. However, you nest /login beneath private page root. Thus you are working around your own auth check code.

摁，如果你把 checkAuth 當作是判斷 private 和 public 的依據來看就會很奇怪。

可是如果他只是會依據權限而有所不同的角度看就不會，另外一種就是在全拆分成這樣：

public
public（會因為有沒有權限而不同）
privae（有權限才可以）

test/Auth/authModule.js, line 15 at r9 (raw file):

Previously, sharils (sharils) wrote…

This test description doesn't really help in terms of understanding what's the purpose of this test. Same as the next test.

got it

Comments from Reviewable

sharils · 2017-02-12T15:25:58Z

Reviewed 3 of 18 files at r1, 5 of 5 files at r7.
Review status: 8 of 20 files reviewed at latest revision, 29 unresolved discussions.

src/Auth/authModule.js, line 11 at r1 (raw file):

如果放在 configureStore 時，當參數越多，這邊相對的也就越長？

You have to choose whether you scatter all these localStorage code across the whole code base or in one place. I guess the answer is obvious.

And that's why I said it's a good reason to look for a middleware to sync the store and localStorage.

如果放在 module 當把一個服務切出去的時候就可以比較乾淨的移出去。

aye

Comments from Reviewable

esbb48 added 14 commits December 24, 2016 20:21

Auth -> App && basic layout

156fca7

Add Login Form

8ba31d2

remove no need .scss

21b325e

add checkAuth

9f868ab

Add loginRequest

b38384f

add onchange for input-username

99f9a26

fix checkAuth and loginRequest

81b887e

Add redux-devtools

e9512a3

Add historyApiFallback = true

fcd177e

Update flow and Add localStorage

4fe4f17

fix bug for username is undefind

7ff1dbf

change mocha to use ava

03d12e8

finish authModule test

797529b

add authSaga

1f6046c

esbb48 force-pushed the feature/3_ali_loginPage branch from 83bc803 to 1f6046c Compare December 31, 2016 05:26

fix bug for rootReducer

8a66db9

sharils requested changes Dec 31, 2016

View reviewed changes

esbb48 added 5 commits January 9, 2017 01:58

remove no need code

be4d02b

Update LoginContainer for rename, refactor, remove fb button

ac697e2

Refactor src/Auth/authModule.js for add and usestage-2, use Symbol

edf5685

refactor

7252898

Refactor test and remove duplicate code and change username

d5e9e66

sharils requested changes Jan 9, 2017

View reviewed changes

esbb48 added 5 commits January 23, 2017 00:54

change stage2 -> transform-object-rest-spread

23d1227

fix bug

5ccf14f

update checkAuth

dbe3cad

Add test for check the username is not hard-code

2678964

refactor

afb014e

sharils requested changes Jan 22, 2017

View reviewed changes

Feature/3 ali login page #11

Are you sure you want to change the base?

Feature/3 ali login page #11

Uh oh!

Conversation

esbb48 commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esbb48 Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esbb48 Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esbb48 Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharils commented Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

esbb48 commented Dec 28, 2016 •

edited

Loading

esbb48 Jan 22, 2017 •

edited

Loading

esbb48 Jan 22, 2017 •

edited

Loading

esbb48 Jan 22, 2017 •

edited

Loading

sharils commented Jan 1, 2017 •

edited

Loading

esbb48 commented Jan 22, 2017 •

edited

Loading