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.

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

Finish main page hero #112

Merged
merged 20 commits into from
Jan 4, 2024
Merged

Finish main page hero #112

merged 20 commits into from
Jan 4, 2024

Conversation

alexzhang1618
Copy link
Contributor

@alexzhang1618 alexzhang1618 commented Jan 2, 2024

Description

What changes did you make? List all distinct problems that this PR addresses. Explain any relevant
motivation or context.

  • Implement the main page hero
  • This also includes check-in handling through the QR code/link, and form on the main page

Things to do:

  • cool and funny animations on the hero

Type of Change

  • Bug Fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)
  • Logistics Change (A change to a README, description, or dev workflow setup like
    linting/formatting)
  • Continuous Integration Change (Related to deployment steps or continuous integration
    workflows)
  • Other: (Fill In)

Testing

I have tested that my changes fully resolve the linked issue ...

  • locally on Desktop.
  • locally on mobile - use https://ngrok.io to get a copy on a mobile device
  • on the live deployment preview on Desktop.
  • on the live deployment preview on Mobile.
  • I have run and passed all new and existing Cypress tests. Add screenshots below.

Checklist

  • I have performed a self-review of my own code.
  • I have followed the style guidelines of this project.
  • I have documented my code's src/lib functions and commented hard to understand areas
    anywhere else.
  • My changes produce no new warnings.

Screenshots

Normal screens
Screenshot 2024-01-02 at 12 24 48 PM
Wide screens
Screenshot 2024-01-02 at 12 25 07 PM
Mobile screens
Screenshot 2024-01-02 at 12 25 28 PM

Copy link

vercel bot commented Jan 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
membership-portal-ui-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 4, 2024 3:54am

Copy link
Member

@farisashai farisashai left a comment

Choose a reason for hiding this comment

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

I really like how you linked every other page from the home page in a good way without needing to use the navbar. I honestly think we should have a "about acm" or "learn more" link in this home section and completely remove the link from the main navigation since it's so insignificant

src/pages/index.tsx Outdated Show resolved Hide resolved
Copy link
Member

@SheepTester SheepTester left a comment

Choose a reason for hiding this comment

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

design suggestions:

image

you could change the padding for .checkin to padding: 1.5rem so it lines up with the link icons

there's a lot of different font sizes in the hero. I think you should make all the smaller font sizes in the hero the same size for consistency. the smaller the better, because having text too big can feel claustrophobic and ruin the hierarchy (people might skim over it like they're headings or ads, and it can be hard to read)

I'd also reduce the line height of the big "Welcome to ACM" text, as well as increasing the spacing between that and the membership points

Also I think you mixed up the store and leaderboard icons

src/components/home/Hero/index.tsx Outdated Show resolved Hide resolved
src/components/home/Hero/style.module.scss Outdated Show resolved Hide resolved
src/components/home/Hero/style.module.scss Outdated Show resolved Hide resolved
src/components/home/Hero/style.module.scss Outdated Show resolved Hide resolved
src/components/home/Hero/style.module.scss Outdated Show resolved Hide resolved
src/pages/index.tsx Outdated Show resolved Hide resolved
src/components/home/Hero/index.tsx Outdated Show resolved Hide resolved
@nishantbalaji
Copy link
Member

nishantbalaji commented Jan 3, 2024

Welcome text / fonts on mobile are really big, pushing more crucial info down into a scroll. Ideally we can reduce size here. Having ACM and the user's name on separate lines looks kinda awkward.

image

@acmucsd acmucsd deleted a comment from nishantbalaji Jan 3, 2024
@alexzhang1618
Copy link
Contributor Author

alexzhang1618 commented Jan 3, 2024

Since the last review:

  • Addressed all the visual/quality feedback
  • Moved the query param check-in to GSSR
    • clear query params using window.history.replaceState
  • Added some code to update the point count and attendance w/o API calls after filling out the check-in form
  • Removed About Us from the navbar and instead added a link on the hero

src/components/home/Hero/index.tsx Outdated Show resolved Hide resolved
src/components/home/Hero/style.module.scss Show resolved Hide resolved
src/pages/index.tsx Outdated Show resolved Hide resolved
src/pages/index.tsx Show resolved Hide resolved
src/pages/index.tsx Show resolved Hide resolved
src/pages/index.tsx Outdated Show resolved Hide resolved
src/pages/index.tsx Show resolved Hide resolved
Copy link
Member

@farisashai farisashai left a comment

Choose a reason for hiding this comment

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

if the typography component is specified as a "a", we should actually be rendering a <Link> component to handle client side routing for us but otherwise changes look good

@alexzhang1618 alexzhang1618 merged commit f06d0df into main Jan 4, 2024
4 checks passed
@alexzhang1618 alexzhang1618 deleted the feature/main-page-hero branch January 4, 2024 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants