React / Redux + better test coverage #145
Conversation
|
default config support added |
| return count; | ||
| }, 0); | ||
|
|
||
| let unreadBadge = !settingsVisible && unreadMessagesCount > 0 ? ( |
There was a problem hiding this comment.
style nit, don't need to wrap all jsx in (). Eg this could be:
let unreadBadge = !settingsVisible && unreadMessagesCount > 0 ?
<div id="sk-badge">
{ unreadMessagesCount }
</div> : null;
There was a problem hiding this comment.
we could, but I'd prefer to either do it nowhere.. or everywhere.. and since it needs to be like that with return ....
| @@ -0,0 +1,80 @@ | |||
| import { ENABLE_SETTINGS, DISABLE_SETTINGS, TOGGLE_WIDGET, OPEN_WIDGET, CLOSE_WIDGET, SHOW_SETTINGS, HIDE_SETTINGS, SHOW_SETTINGS_NOTIFICATION, HIDE_SETTINGS_NOTIFICATION, SET_SERVER_URL, SET_EMAIL_READONLY, UNSET_EMAIL_READONLY, UPDATE_READ_TIMESTAMP } from 'actions/app-state-actions'; | |||
There was a problem hiding this comment.
import * as AppStateActions from 'actions/app-state-actions' and do AppStateActions.UNSET_EMAIL_READONLY everywhere.. the formatter won't let me put that on mutliple lines
There was a problem hiding this comment.
why do action consts need to be in separate files?
what if we had a single actions.js one to rule them all
then you could import * as action from 'actions'
There was a problem hiding this comment.
it's not just the constants in those files, there are also the action creators functions
| } | ||
| } | ||
|
|
||
| trigger(event, args) { |
There was a problem hiding this comment.
I think perhaps you want:
trigger(event, ...args) {
and
map.get(event).forEach(handler => handler(...args));
There was a problem hiding this comment.
maybe it shouldn't be called args.. I thought about doing that, but the current eventing system (backbone events) use (event, options)
| @@ -0,0 +1,121 @@ | |||
| import sinon from 'sinon'; | |||
There was a problem hiding this comment.
Hmm. Tests seem way nicer with react... :)
| describe('Header', () => { | ||
| afterEach(() => { | ||
| sandbox.restore(); | ||
| defaultProps.actions.showSettings = sandbox.spy(); |
There was a problem hiding this comment.
What if we change const defaultProps to let props and initialize it in a beforeEach like this:
beforeEach( () => {
props = {
...
};
});
You wouldn't need to reset the spies in this afterEach and it would reduce the possibility of tests interfering with another.
|
@dannytranlx tag line position fixed |
|
Press CTRL + D to Bookmark this page and get quick access to your favorite ASCII Art. ╱╱┏╮ ▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒ ────────────────────░███░ ★░░░░░░░░░░░████░░░░░░░░░░░░░░░░░░░░★ ★░░░░░░░░░███░██░░░░░░░░░░░░░░░░░░░░★ ★░░░░░░░░░██░░░█░░░░░░░░░░░░░░░░░░░░★ ★░░░░░░░░░██░░░██░░░░░░░░░░░░░░░░░░░★ ★░░░░░░░░░░██░░░███░░░░░░░░░░░░░░░░░★ ★░░░░░░░░░░░██░░░░██░░░░░░░░░░░░░░░░★ ★░░░░░░░░░░░██░░░░░███░░░░░░░░░░░░░░★ ★░░░░░░░░░░░░██░░░░░░██░░░░░░░░░░░░░★ ★░░░░░░░███████░░░░░░░██░░░░░░░░░░░░★ ★░░░░█████░░░░░░░░░░░░░░███░██░░░░░░★ ★░░░██░░░░░████░░░░░░░░░░██████░░░░░★ ★░░░██░░████░░███░░░░░░░░░░░░░██░░░░★ ★░░░██░░░░░░░░███░░░░░░░░░░░░░██░░░░★ ★░░░░██████████░███░░░░░░░░░░░██░░░░★ ★░░░░██░░░░░░░░████░░░░░░░░░░░██░░░░★ ★░░░░███████████░░██░░░░░░░░░░██░░░░★ ★░░░░░░██░░░░░░░████░░░░░██████░░░░░★ ★░░░░░░██████████░██░░░░███░██░░░░░░★ ★░░░░░░░░░██░░░░░████░███░░░░░░░░░░░★ ★░░░░░░░░░█████████████░░░░░░░░░░░░░★ ★░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░★ ┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌┌ |
React / Redux + better test coverage
|
awesome. and most of the tests look so much tighter then with backbone |
This is a big chunk of changes to move away from Marionette and move towards React.
This PR includes a bigger test suite that covers almost everything in the widget.
It also leverages https://github.com/lemieux/smooch-core-js to talk with our API.
Happy reviewing @alavers @jpjoyal @dannytranlx @jugarrit @mspensieri @Mario54 @spasiu :)