-
Notifications
You must be signed in to change notification settings - Fork 46
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
New home page blockers #11338
New home page blockers #11338
Conversation
… into salman/issue#111281/new-home-page-blocker
@@ -63,12 +63,12 @@ const ActiveContestList = ({ | |||
<div className="ActiveContestList"> | |||
<div className="heading-container"> | |||
<CWText type="h2">Contests</CWText> | |||
<Link to="/explore"> | |||
<HashLink smooth to="/explore#contests"> |
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.
What's the purpose of using HashLink instead of the normal Link? Are we transitioning to use this now? I don't see this used anywhere else in the codebase.
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.
@Israellund I used HashLink as a neat wrapper for scrolling—it auto-handles smooth scrolling via the URL hash, i fealt like its more cleaner way of doing this. But I'm happy to switch to a manual ref if preferred!
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.
@salman-neslit Yeah, that makes sense in regards to implementation. @masvelio @mzparacha Do you have any thoughts on this?
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 don't have any objections, seems to improve UX. Marcin wdyt?
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.
smooth is good for sure, question if we need to install 3rd party for this specific use case?
window.scrollTo({
top: element.offsetTop,
behavior: 'smooth',
});
We use it already in the app, wouldn't this work?
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.
@masvelio I opted for this package approach as this keeps the implementation cleaner. Additionally, behavior: 'smooth' was inconsistent sometimes it worked, but other times the scrolling was abrupt. That said, I'm happy to go with this approach if you prefer?
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.
Approving this now but you might want to wait to see what Marcin says before merging
we can safely close this it's been superseded |
Link to Issue
Closes: #11281
Description of Changes
"How We Fixed It"
Test Plan
Deployment Plan
Other Considerations