Fix disabled progress buttons, add tests#25
Conversation
…web/bugfix/progress-disabled
|
Reminder on this @chamkank @mingyokim |
mingyokim
left a comment
There was a problem hiding this comment.
some minor comments on tests, otherwise looks good! tried it on my local as well
| }); | ||
|
|
||
| test('active button should have class "active"', () => { | ||
| expect(wrapper.childAt(activeIndex).hasClass('active')).toBeTruthy(); |
There was a problem hiding this comment.
We could also iterate through all the buttons to check that the non-active buttons don't have the class active
|
|
||
| describe('when some steps are disabled', () => { | ||
| const count = 4; | ||
| const lastActiveIndex = 4; |
There was a problem hiding this comment.
I'm confused... why is lastActiveIndex 4? The max it can be should be 3 based on count
There was a problem hiding this comment.
I don't think it's very important - makes it easy to enable all buttons by setting lastActiveIndex as 999
There was a problem hiding this comment.
oh yeah but i think this test is trying to test when some steps are disabled - so why is it enabling all buttons?
There was a problem hiding this comment.
oh wait, this isn't used at all:
wrapper = getWrapper({
count,
onClick: (i) => { clicked = i; },
activeIndex: 1,
lastActiveIndex: count / 2,
});ill change this oops
| for (let i = 0; i < count; i += 1) { | ||
| wrapper.childAt(i).simulate('click'); | ||
| if (i <= lastActiveIndex) { | ||
| expect(clicked).toEqual(i); |
There was a problem hiding this comment.
we could also test that the component has disabled property, especially since "Enzyme click simulations do not respect 'disabled' attribute" as you've mentioned.
There was a problem hiding this comment.
that's on the next line:
if (i <= lastActiveIndex) {
expect(clicked).toEqual(i);
} else {
expect(wrapper.childAt(i).props().disabled).toBeTruthy();
// Enzyme click simulations do not respect 'disabled' attribute.
// See https://github.com/airbnb/enzyme/issues/386
// expect(clicked).not.toEqual(i)
}There was a problem hiding this comment.
oh sorry, i meant to test that the component does NOT have disabled property when it's less than lastActiveIndex
i.e. expect(wrapper.childAt(i).props().disabled).toBeFalsy();
…web/bugfix/progress-disabled
| for (let i = 0; i < count; i += 1) { | ||
| wrapper.find('button').at(i).simulate('click'); | ||
| if (i <= lastValidIndex) { | ||
| expect(clicked).toEqual(i); |
There was a problem hiding this comment.
can we test that the prop does not have disabled property when it's less than lastActiveIndex?
i.e. expect(wrapper.childAt(i).props().disabled).toBeFalsy();
I think it's especially important here since enzyme click simulations don't work with disabled attribute - onClick can be triggered even if the button is disabled, so testing that onClick is triggered by itself would not guarantee that the button is not disabled
mingyokim
left a comment
There was a problem hiding this comment.
Sorry for the delay, but looks good!!
|
Thanks for the feedback @mingyokim |
👷 Changes
Fix disabled progress buttons, add tests - still WIP
💭 Notes
n/a
🔦 Testing Instructions