-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: adds gap property #29
base: main
Are you sure you want to change the base?
Conversation
this closes #23 |
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.
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
@@ -3,6 +3,7 @@ | |||
export let align = 'center'; | |||
export let justify = 'center'; | |||
export let reverse = false; | |||
export let gap = '0'; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think that leaving as null / undefined would be better (because it wouldnt be applied)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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