make sure stepCount doesn't count false / null or undefined child items #106
Conversation
|
Thanks for this PR, really appreciate it! |
|
Hah, just came to report this as I got hit by this behavior and had read up on the differences between TIL 😀 Nice to see it's already been fixed — thank you! |
| @@ -37,6 +38,20 @@ describe('useWizard', () => { | |||
| expect(result.current.stepCount).toBe(2); | |||
| }); | |||
|
|
|||
| test('should set step count to one when using falsy step', () => { | |||
There was a problem hiding this comment.
@devrnt Had a quick review and spot two things:
- Isn't the behavior tested twice now? An optional step is already added in
renderUseWizardHook(). Hence, the test above ('should set step count to two') should already capture this? - If you still prefer to have a separate (personally, I would just remove this test), more explicit test, then the name of the test doesn't reflect the implementation of the test.
2.1 It should either sayset step count to _two_or one step should be removed.
2.2 Extreme nitpick, but "falsy" isn't entirely true. E.g.""and0would render. I would call it "non-rendered" or something.
There was a problem hiding this comment.
You're absolutely right, "falsy" is wrong here, don't know why I put it there :)
Anyway I changed the test case, if you're interested you can check it out
There was a problem hiding this comment.
All right, that's better 👍
I'd still argue the optional step isn't needed in the default render hook test wrapper, but no biggie.
When using optional steps in your Wizard the new stepCount property didn't match.
This is because
React.Children.count()will also count the null or false values in jsx.When converting to children toArray only valid JSX is allowed so we get the actual child count.
For more info on this: https://blog.agney.dev/react-children-count/