Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#12 calendar component #44

Open
wants to merge 5 commits into
base: staging
Choose a base branch
from
Open

Conversation

michael-alade
Copy link

@michael-alade michael-alade commented Jan 16, 2017

Description

This PR solves the issue for creating a date-selector component
Create DateSelector component

Purpose

To give the ability to view the menu by date

Solved Issue

#12
#15

@michael-alade michael-alade requested review from ruqoyyasadiq and a user January 16, 2017 22:02
@michael-alade michael-alade self-assigned this Jan 16, 2017
import Calendar from './calendar';
import { CalendarIcon, ExpandIcon } from '../icons';

describe('<Calendar />', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this tests are not comprehensive enough. How do you test that this component has multiple ui classes
how do you test that it contains span, how do you test how many divs it has? how do you test for the text it contains?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@andela-akolawole please make sure to deep-test your components just like @abiodun0 has pointed out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andela-akolawole Nice work so far. However, these tests do not seem comprehensive enough. How about adding tests to check that this component has multiple ui classes, contains span, number of divs it has and the text it contains.

for example. we can write tests for classes like this

expect(wrapper.find('.ui')).to.have.length(3);
expect(wrapper.find('.tab')).to.have.length(1);
expect(wrapper.find('span')).to.have.length(1);

Remember the purpose of tests is to be able to look at the test and know what the code is doing. A better way of communicating what you are trying to achieve to the other developer so it should be comprehensive enough.

Copy link
Contributor

@abiodun0 abiodun0 left a comment

Choose a reason for hiding this comment

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

Write more comprehensive tests!

@kosyfrances
Copy link
Member

@andela-akolawole please correct me if I am wrong. My assumption is that this PR also solves issue #15 right? I'm trying to get the trello board and issues in sync

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling de4a0d2 on 15_calendar_components into ** on staging**.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.0% when pulling a674a3b on 15_calendar_components into b747977 on staging.

@michael-alade
Copy link
Author

@abiodun0 I just finished writing the test

@michael-alade michael-alade changed the title #15 calendar component #12 calendar component Feb 9, 2017
Copy link
Contributor

@abiodun0 abiodun0 left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

👍

@michael-alade michael-alade force-pushed the 15_calendar_components branch from a674a3b to bcb5814 Compare March 29, 2017 08:28
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.

5 participants