-
Notifications
You must be signed in to change notification settings - Fork 1
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
Yap review #8
Comments
Hey, thank you for taking the time - banger of a review! Let me get back to you on some of these
Thanks again, this sets me off in a really nice direction for some improvements for a big new iteration. |
This component is a good canditate to try out the ts-pattern package for states
selecta/src/provider/SignInProvider.tsx
Line 26 in 5addba3
Add prettier or biome to help you with format consistency, I see some of these are single quotes and some are double quotes
selecta/src/provider/AuthProvider.tsx
Line 1 in 5addba3
For writing test descriptions, I personally look at it as the "should" between
it
and what goes in(...)
is silent i.e. the statement "getSessionToken should return a valid token" in test form could be written asdescribe("getSessionToken")
next line will beit("return a valid token")
so between the describe and it, there's a silent 'should' which somewhat makes the test read betterselecta/src/lib/utils.test.ts
Line 60 in 5addba3
ofetch from unjs.io is much much better than the polyfilled fetch out there. Just sayin
selecta/src/lib/spotify/utils.ts
Line 55 in 5addba3
just
selecta/src/lib/spotify/constants.ts
Line 42 in 5addba3
Why do react people do this? This component has a single node already, what's the point of the empty tags?
selecta/src/components/loading/LoadingTracks.tsx
Line 3 in 5addba3
I got to use it a bit, it's pretty neat. It took a while to figure out how to get started but that's a story for another day
The text was updated successfully, but these errors were encountered: