Skip to content
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(types): Added types in useApiCall hooks #529

Merged
merged 6 commits into from
May 20, 2024
Merged

feat(types): Added types in useApiCall hooks #529

merged 6 commits into from
May 20, 2024

Conversation

Gmin2
Copy link
Contributor

@Gmin2 Gmin2 commented Apr 26, 2024

Completed the migration to typescript for all the API calls in pages .

@Gmin2 Gmin2 requested a review from frouioui as a code owner April 26, 2024 08:06
Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall the changes proposed in this Pull Request look good to me, I left two nits. I see in the PR description you want to add type definition for the compare page, let's do this. Moreover, could you go over the FK and Daily pages? That way we will be using proper types in every page.

Only page left would be the query comparison page, but this can be left for later, the API for that page has to change a little bit too anyway.

Not approving this as you're planning on doing more work, but looks great!

website/src/types/index.ts Outdated Show resolved Hide resolved
website/src/pages/SearchPage/SearchPage.tsx Show resolved Hide resolved
@frouioui
Copy link
Member

After merging #538 there are some conflicts on this branch.

@Gmin2
Copy link
Contributor Author

Gmin2 commented Apr 30, 2024

After merging #538 there are some conflicts on this branch.

My end exams semester are going on will be resolving it after the week

@frouioui
Copy link
Member

My end exams semester are going on will be resolving it after the week

No worries at all, take your time and good luck.

@Gmin2
Copy link
Contributor Author

Gmin2 commented May 7, 2024

Ready to review @frouioui

@Gmin2
Copy link
Contributor Author

Gmin2 commented May 10, 2024

@frouioui can we merge this soon, its types are very important to migrate other component

@frouioui frouioui merged commit fda5237 into vitessio:main May 20, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants