Skip to content

Conversation

@erikaperugachi
Copy link
Contributor

No description provided.

@GianniCarlo GianniCarlo requested a review from not-gabriel June 13, 2018 03:40
dispatch(updateLabelSuccess(label));
}
} catch (e) {
// To do
Copy link
Contributor

Choose a reason for hiding this comment

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

you should really start working on a general error handler instead of sprinkling todos all over your codebase

it('should update label: less badge', () => {
const data = Map({
[labels[0].id]: Map({
id: labels[0].id,
Copy link
Contributor

Choose a reason for hiding this comment

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

use const firstLabel = labels[0]

Copy link
Contributor Author

@erikaperugachi erikaperugachi Jun 13, 2018

Choose a reason for hiding this comment

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

In the test I don't mind if it is the first, second, whatever label.... it just take a label to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

so what?it is still repetitive. use const [anyLabel] = labels

});
const action = actions.updateLabelSuccess({ id: 1, badgeOperation: -1 });
const state = labelReducer(data, action);
expect(state).toMatchSnapshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

dont abuse snapshots, the expect API has more appropriate methods for this

const action = actions.updateLabelSuccess({ id: 1, badgeOperation });
const state = labelReducer(data, action);
expect(state.get('1').get('badge')).toEqual(
data.get('1').get('badge') + badgeOperation
Copy link
Contributor

Choose a reason for hiding this comment

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

this is hard to read. why not use expect toEqual arrayContaining expectedBadge.

it('should update label: less badge', () => {
const data = Map({
[labels[0].id]: Map({
id: labels[0].id,
Copy link
Contributor

Choose a reason for hiding this comment

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

so what?it is still repetitive. use const [anyLabel] = labels

@not-gabriel not-gabriel merged commit a6241d0 into Criptext:master Jun 13, 2018
@erikaperugachi erikaperugachi deleted the update-badge branch June 13, 2018 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants