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.

Sign up for GitHub

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

Jump to bottom

feat(client): ts-migrate WithInstantSearch.js #42923

Merged
merged 7 commits into from
Aug 5, 2021
Merged

feat(client): ts-migrate WithInstantSearch.js #42923

merged 7 commits into from
Aug 5, 2021

Conversation

awu43
Copy link
Contributor

@awu43 awu43 commented Jul 18, 2021 •
edited
Loading

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the main branch of freeCodeCamp.
  • I have tested these changes either locally on my machine, or GitPod.

Refers #42256

Converted InstantSearchRoot to functional component.

@awu43 awu43 requested a review from a team as a code owner July 18, 2021 07:12
@gitpod-io
Copy link

gitpod-io bot commented Jul 18, 2021

@github-actions github-actions bot added the platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. label Jul 18, 2021
ShaunSHamilton
ShaunSHamilton reviewed Jul 18, 2021
View reviewed changes
Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

Hey there. Thanks again, for another contribution.

I am just double checking, but do you follow the suggestion on the issue for how to rename files and keep their history?:

Keeping Git History
Sometimes changing the file from .js -> .tsx? causes the original file to be deleted, and a new one created, and other times the filename just changes - in terms of Git. Ideally, we want the file history to be preserved.

The best bet at achieving this is by:

  1. Rename the file
  2. Commit with the flag --no-verify to prevent husky from complaining about the errors
  3. Refactor to TS for migration, in a separate commit.

Note: Editors like VSCode are still likely to show you the file has been deleted and a new one created. If you use the CLI to git add ., then VSCode will show the file as renamed in stage

@awu43
Copy link
Contributor Author

awu43 commented Jul 18, 2021

I am just double checking, but do you follow the suggestion on the issue for how to rename files and keep their history?

Yes, the first commit is for renaming files only with no content changes. What's strange is that Git correctly identifies the renaming commit and the migrating commit individually, but not together in Files changed.

@ShaunSHamilton
Copy link
Member

I am just double checking, but do you follow the suggestion on the issue for how to rename files and keep their history?

Yes, the first commit is for renaming files only with no content changes. What's strange is that Git correctly identifies the renaming commit and the migrating commit individually, but not together in Files changed.

Thanks, for confirming. It is just odd why it sometimes works.

@ojeytonwilliams
Copy link
Contributor

It is a bit odd. For what it's worth, Git Graph is still very readable, as is the last commit.

ojeytonwilliams
ojeytonwilliams requested changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

I just want to address the algolia id/key issue first, so that we can get past the linting step.

client/src/components/search/with-instant-search.tsx Show resolved Hide resolved
@gikf gikf added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Jul 25, 2021
@moT01 moT01 added status: merge conflict To be applied to PR's that have a merge conflict and need updating and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Aug 2, 2021
ojeytonwilliams
ojeytonwilliams requested changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the conflicts, @awu43

This PR does look good, but one general observation: it's difficult to properly review (and potentially debug) PRs that do multiple things at once. In this case, we can't simply look at the diff to see which changes are just part of the the TS migration and which are caused by the change to use functional components.

That aside, there's one nitpick below:

client/src/components/search/with-instant-search.tsx Outdated Show resolved Hide resolved
client/src/components/search/with-instant-search.tsx Outdated Show resolved Hide resolved
@ojeytonwilliams ojeytonwilliams added status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP and removed status: merge conflict To be applied to PR's that have a merge conflict and need updating labels Aug 4, 2021
@awu43
Copy link
Contributor Author

awu43 commented Aug 5, 2021

Good to know about the component types, I'll keep it in mind.

ojeytonwilliams
ojeytonwilliams approved these changes Aug 5, 2021
View reviewed changes
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks @awu43 this looks solid.

@ShaunSHamilton ShaunSHamilton added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels Aug 5, 2021
ShaunSHamilton
ShaunSHamilton approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

This LGTM 👍

@ShaunSHamilton ShaunSHamilton merged commit 4134404 into freeCodeCamp:main Aug 5, 2021
@ShaunSHamilton
Copy link
Member

Thank you, for your contribution to the page! 👍
We are happy to accept these changes ,and look forward to future contributions. 🎉

@awu43 awu43 deleted the feat/ts-migrate-with-instance-search branch August 6, 2021 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaunSHamilton ShaunSHamilton ShaunSHamilton approved these changes

@ojeytonwilliams ojeytonwilliams ojeytonwilliams approved these changes

Assignees
No one assigned
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@awu43 @ShaunSHamilton @ojeytonwilliams @moT01 @gikf