-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: staging
Are you sure you want to change the base?
Conversation
import Calendar from './calendar'; | ||
import { CalendarIcon, ExpandIcon } from '../icons'; | ||
|
||
describe('<Calendar />', () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
@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 |
Changes Unknown when pulling de4a0d2 on 15_calendar_components into ** on staging**. |
@abiodun0 I just finished writing the test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
a674a3b
to
bcb5814
Compare
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