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

Hyperlink scroll #198

Merged
merged 6 commits into from
Feb 14, 2022
Merged

Hyperlink scroll #198

merged 6 commits into from
Feb 14, 2022

Conversation

farisashai
Copy link
Member

  • fixed scrolling on community page to offset top margin by header height so active section doesn't appear under the header
  • fixed weird bug at exactly 900px where mobile navbar would disappear if opened because of overlapping media queries
  • simplified scroll to contact (had a weird onClick function that manually scrolled the window because nobody ever gave the footer an id)

still not sure why links from community grid go to wrong location sometimes when we're clicking from a diff page

@farisashai farisashai self-assigned this Feb 13, 2022
@vercel
Copy link

vercel bot commented Feb 13, 2022 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/acmucsd/main-website/2ZL3sD7ZFmbzR4d61Uc9qBeCAQCy
✅ Preview: https://main-website-git-hyperlink-scroll-acmucsd.vercel.app

trulyronak
trulyronak suggested changes Feb 13, 2022
View reviewed changes
Copy link
Member

@trulyronak trulyronak left a comment

Choose a reason for hiding this comment

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

Can you also make it so the mobile navbar goes away after you click on a link

Also if possible can you add all the media query values to a global vars CSS file (just to make it a bit cleaner)

src/components/NavigationBar/style.scss Outdated Show resolved Hide resolved
@trulyronak
Copy link
Member

Moved the media queries request to #202 since it's a bigger problem that I thought

trulyronak
trulyronak approved these changes Feb 13, 2022
View reviewed changes
Copy link
Member

@trulyronak trulyronak left a comment

Choose a reason for hiding this comment

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

Hype

trulyronak
trulyronak approved these changes Feb 14, 2022
View reviewed changes
Copy link
Member

@trulyronak trulyronak left a comment

Choose a reason for hiding this comment

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

Looks good, a bit curious why we don't need scrollToContacts

src/components/NavigationBar/index.tsx
Comment on lines -19 to -22
const scrollToContacts = (): void => {
window.scrollTo({ top: document.body.scrollHeight });
setMenuState(false);
};
Copy link
Member

Choose a reason for hiding this comment

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

seeking to learn: why did you remove scrollToContacts

Copy link
Member Author

Choose a reason for hiding this comment

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

the 2 things happening here are manually forcing the page to scroll and closing the mobile nav menu. we no longer need to force the page to scroll because the link href works as intended by giving the footer an id. closing the mobile nav menu is handled by toggleMenu because the only time it gets called is when menuState is true

@farisashai farisashai merged commit 24c03aa into main Feb 14, 2022
@farisashai farisashai deleted the hyperlink-scroll branch February 14, 2022 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trulyronak trulyronak trulyronak approved these changes

Assignees

@farisashai farisashai

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@farisashai @trulyronak