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

Replace ScrollToTop with useScrollToTop #3034

Closed
taneliang opened this issue Dec 7, 2020 · 2 comments · Fixed by #3041
Closed

Replace ScrollToTop with useScrollToTop #3034

taneliang opened this issue Dec 7, 2020 · 2 comments · Fixed by #3041
Assignees
@teikjun
Labels
better engineering good first issue taken

Comments

@taneliang
Copy link
Member

useScrollToTop was added in #3007 to replace the ScrollToTop component. It's currently still used in 2 class components (ModulePageContent and SettingsContainer), which need to be converted to functional components before we can delete ScrollToTop.

This issue exists so that we remember to remove ScrollToTop the codebase.

Steps

  1. Convert ModulePageContent and/or SettingsContainer to function components (both should be easy to convert).
  2. Replace <ScrollToTop /> with a useScrollToTop() call.
  3. Delete ScrollToTop from the codebase.
@teikjun
Copy link
Contributor

teikjun commented Dec 7, 2020

I can work on it! :D

@taneliang
Copy link
Member Author

Sure, thank you! :D

@taneliang taneliang added the taken label Dec 8, 2020
@teikjun teikjun mentioned this issue Dec 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@teikjun teikjun

Labels
better engineering good first issue taken
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace ScrollToTop with useScrollToTop teikjun/nusmods
2 participants
@taneliang @teikjun