Conversation
|
this closes #23 |
himynameisdave
left a comment
himynameisdave
left a comment
There was a problem hiding this comment.
Thanks for contributing! For your first time writing tests, they were great! 👍 💯
Please see my comments about using a better default value. Once that is cleared up, I can approve and merge this one (and move onto #30 too!)
src/Flex.svelte
Outdated
| export let align = 'center'; | ||
| export let justify = 'center'; | ||
| export let reverse = false; | ||
| export let gap = '0'; |
There was a problem hiding this comment.
This should be a number, not a string.
| export let gap = '0'; | |
| export let gap = 0; |
Eventually I'd like to add Typescript definitions for this package, which would help make this more clear.
There was a problem hiding this comment.
Actually, I think this should be the initial value for gap, which I think is 'normal normal' (based on MDN). Either that or we should perhaps leave it undefined/null or something, as I don't want it to be applied if consumers aren't using it.
There was a problem hiding this comment.
i think that leaving as null / undefined would be better (because it wouldnt be applied)
There was a problem hiding this comment.
Yes let's leave it as null or undefined
| test('bad value', () => { | ||
| const container = renderFlex({ gap: 'oops' }); | ||
| expect(container).not.toHaveStyle('gap: 1rem'); | ||
| expect(container).not.toHaveStyle('gap: 0'); |
There was a problem hiding this comment.
Great tests! 😄
Perhaps we can also add an expected fallback/default value? (the initial value we discussed above).
There was a problem hiding this comment.
how can i test this? as far as i know, if i make the default value for the gap prop equal to undefined, the svelte style directive would just ignore it and not pass anything.
should i expect(container).not.toHaveStyle('gap: 0')?
There was a problem hiding this comment.
Because the initial value for gap is 'normal normal', I think it would be a great idea to do a positive check for that value (I think that testing-library's toHaveStyle should return true because it is using getComputedStyle under the hood, which always returns whatever the initial value is if none is set.
Try this:
expect(container).toHaveStyle({ gap: 'normal normal' });If not, I'd say the tests you've already added are sufficient.
There was a problem hiding this comment.
tried expecting it to have gap: 'normal normal' but it didnt work. im going to remove the test for "does not has gap prop", but feel free to add it after merging the PR. also, pls check my comment on #30
|
@demetrius-mp let me know when you've made those changes, happy to approve and move this along! |
@himynameisdave hi! i think i have it all working, just check my comment on the other PR to make sure it has everything you asked for! |
|
i think now everything is ok @himynameisdave |
|
@demetrius-mp sorry to do this to you, but I've updated this to take advantage of Sveltekit's packaging feature. You may need to rebase before this will work. Sorry for the churn! |
2 participants
please, check the tests, this is my first time writing tests
i saw that you always do a "default / fallback" test, but i couldn't figure how to do it with this prop