Restore User announce post body & first login UI fix

moneybutton / yours-core

Client-side key and content management for Yours.

https://www.yours.network

MIT License

132 stars 27 forks source link

Restore User announce post body & first login UI fix #93

Closed willy-b closed 8 years ago

willy-b commented 8 years ago

restore User announce post body
fix "no content" popping up before signup wizard
do not switch to '#/setup' during test
all tests passing

willy-b commented 8 years ago

Merged magic hash changes, ready for merge to master.

ryanxcharles commented 8 years ago

Great, Willy. Some issues:

The tests don't pass - there are "js standard" issues. Run "npm test".
Your commit titles are too long. Commit titles should be limited to 50 chars. If you need to write more, put a blank line after the title, and put the rest of the message below.
See the two inline comments.

willy-b commented 8 years ago

Separated UI events to UI specific EventEmitter (iterating towards Flux), and fixed tests. Also got rid of 2 second delay for UI refresh and replaced it with an event-based solution. Let me know if it needs anything else for merge!

ryanxcharles commented 8 years ago

Comments that don't prevent a merge:

Do not merge from master, especially not more than once. This will create a nonlinear history that is difficult to understand. You should be rebasing your PRs on top of master. The only merge commits that exist should be merging into master.
We should be using Redux instead of events. After inquiring with @maraoz, he agrees and argues Redux is better than events :)

ryanxcharles commented 8 years ago

Also, your commit messages should be no wider than 80 chars. Your editor should have a way to limit the width to 80 chars. See how this looks non-wrapped when viewing with default settings with 'git log':

screenshot from 2016-02-23 14 00 50

Contrast that with an earlier commit made by me which does wrap at 80 chars:

screenshot from 2016-02-23 14 03 31

ryanxcharles commented 8 years ago

I get a test failure when running the tests. This prevents a merge.

  208 passing (15s)
  1 pending
  1 failing
  1) Layout "after all" hook:
     TypeError: Cannot read property 'unmonitorBlockchainAPI' of undefined
      at DattCore. (index.js:177:5)
      at [object Generator].next (native)
      at node_modules/asink/index.js:47:35
      at step (node_modules/asink/index.js:29:16)
      at node_modules/asink/index.js:47:5
      at new Promise (node_modules/karma/node_modules/core-js/modules/es6.promise.js:209:7)
      at spawn (node_modules/asink/index.js:24:10)
      at DattCore.asyncNetworkClose (index.js:176:10)
      at layout.jsx:28:22
      at [object Generator].next (native)
      at node_modules/asink/index.js:47:35
      at step (node_modules/asink/index.js:29:16)
      at node_modules/asink/index.js:47:5
      at new Promise (node_modules/karma/node_modules/core-js/modules/es6.promise.js:209:7)
      at spawn (node_modules/asink/index.js:24:10)
      at Context. (layout.jsx:27:12)

ryanxcharles commented 8 years ago

Project has been rearchitected - PR no longer applies.