-
Notifications
You must be signed in to change notification settings - Fork 319
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
Autoscroll Course Planner to current year card #3640
base: master
Are you sure you want to change the base?
Autoscroll Course Planner to current year card #3640
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@jacobkwan is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel. @nusmodifications first needs to authorize it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3640 +/- ##
==========================================
- Coverage 53.73% 53.63% -0.10%
==========================================
Files 272 272
Lines 5959 5968 +9
Branches 1418 1421 +3
==========================================
- Hits 3202 3201 -1
- Misses 2757 2767 +10 ☔ View full report in Codecov by Sentry. |
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.
I think this is a good idea, although ref is used incorrectly here
@@ -24,14 +24,31 @@ type Props = Readonly<{ | |||
|
|||
type State = { | |||
readonly showSpecialSem: boolean; | |||
readonly currentYearCardRef: Ref<HTMLDivElement>; |
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.
ref should not be stored in state, see https://react.dev/reference/react/createRef
} | ||
parentContainer.scrollTo({ | ||
left: currentYearCard.offsetLeft - parentContainer.offsetLeft, | ||
behavior: 'smooth', |
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.
For scrolling on component mount you probably don't want smooth, it's better for the component to appear already scrolled to the correct location.
if (!currentYearCard || !parentContainer) { | ||
return; | ||
} | ||
parentContainer.scrollTo({ |
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.
Could consider using scrollIntoView
instead, that would avoid having to do math to figure out exact offset
Context
#3639: Autoscroll Course Planner to current year on first render
Implementation
currentYearCardRef
state variablecurrentYearCardRef
to thePlannerYear
card for the current year of enrolmentcomponentDidMount
method inPlannerYear
currentYearCardRef
to get parent container and calculate offsets for child and parentsmooth
effect)Before
Screen.Recording.2023-11-09.at.10.40.21.PM.mov
After
Screen.Recording.2023-11-09.at.10.49.25.PM.mov
Other Information