-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ref(js): Introduce simple useProjects #29457
ref(js): Introduce simple useProjects #29457
Conversation
size-limit report
|
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.
niceee
This is super basic right now, a bunch of tests are failing that I'm fixing, then right after this we'll have all the project util stuff end up in here 😎 |
01d5558
to
aadfc53
Compare
aadfc53
to
117e6df
Compare
117e6df
to
6e3ab87
Compare
@priscilawebdev @matejminar could I get a review for all the tests I changed. Because withProjects uses the hook now it needed a lot more |
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.
nice stuff, and good updates to RTL over multiple await ticks
expect(initialData.router.replace).toHaveBeenLastCalledWith({ | ||
pathname: undefined, | ||
query: {environment: [], project: [0]}, | ||
query: {cursor: undefined, environment: [], project: [2]}, |
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.
is it bad that we changed the functionality here?
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.
So the reason this changed is a detail of the test implementation because we had mocked the return value incorrectly in the test before.
So it's actually the same 👍
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.
ahhh i see now
Switches the withProjects over to the useProjects hook, which very basically just connects to the store ATM