-
Notifications
You must be signed in to change notification settings - Fork 4
test #189: create unit test for gift suggestion card #624
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
base: develop
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| * @param {string} giftSuggestionParams.budget - the exchange budget for gift. | ||
| * @param {IGiftSuggestion} giftSuggestionParams.gift - the gift suggestion. | ||
| * @param {number} giftSuggestionParams.index - the index of the gift. | ||
| * @param {IGiftSuggestion | number} giftSuggestionParams.onGiftUpdate - the updated value of index and gift suggestion. |
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.
The type here: {IGiftSuggestion | number} doesn't seem right for a handler function.
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 was not sure what to put for that function so placed the types of its params.
When I looked at the documentation the params that seemed to align the most would Callback Function https://jsdoc.app/tags-param (bottom of page). Do you agree or do you think something else would be more fitting?
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.
Yeah, was looking at the same docs earlier and thought the same about cb being the most similar example in the docs. But looking at other examples from around the interwebs I'm seeing a lot more of this:
* @param {function} props.onGiftUpdate - A function that does x,y,z
or maybe
* @param {function(IGiftSuggestion, number): void} props.onGiftUpdate - ...
I don't see any examples elsewhere in our codebase, so I'll be interested to see what Seniors have to say on this one...
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 was hesitant to put function as the type because I had never seen that be used but I can guinea pig it
Description
Create unit test for gift suggestion card
Before:
Previously, component had no unit test
After:
Describe new behavior, including what was changed and why
Closes #189
Testing instructions
If applicable, provide steps for reviewers to test changes locally -- including necessary setup, commands, and expected results
Additional information
Share any additional info that may provide context for the PR evaluation (performance considerations, design choices, etc)
[optional] Screenshots
Pre-submission checklist
test #001: created unit test for __ component)Peer Code ReviewersandSenior+ Code Reviewersgroupsgis-code-questions