Skip to content

Miscellaneous storefront nitpicks #84

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

Merged
merged 57 commits into from
Nov 11, 2023
Merged

Miscellaneous storefront nitpicks #84

merged 57 commits into from
Nov 11, 2023

Conversation

SheepTester
Copy link
Member

Info

Closes #79.

Description

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

See the checked boxes in #79, or Changes

Attempts to add a more robust store navbar back button

Here's a list of attempts to add a back button that works better than just () => router.back(). I tried to fix it in this PR but ended up not, so this serves as a record for anyone trying to fix the back button in the future.

  • The initial idea was to have each page specify a route for the back button to return to. For example, a collection page could link back to /store, and the storefront page could just not have a back button.

    Downsides: Since items are directly linked to from the storefront page and the collection page, it's not clear whether items should link to the collection page or the storefront page, since it wouldn't be able to know how the user got to the item page. It also adds a new entry to the browser history, so using the browser back button undoes the navbar's back button (ref)

  • Currently, the navbar just uses router.back(), and the storefront page hides the back button.

    Downsides: If you directly visit any other store page, then the back button will kick you off the ACM website or not do anything.

Ideally, the navbar would know whether the previous page is under /store, but this is harder than it sounds:

  • document.referrer

    Downsides: Next.JS changes the page contents without reloading the page, so document.referrer doesn't change when navigating between pages.

  • Keeping track of a history of routes, like this:

    const [history, setHistory] = useState([]);
    router.on('route', newRoute => {
      setHistory([...history, newRoute]);
    });

    It's a bit of a pain with Next.JS. Also, it's probably very finicky. All the browser's native History API tells you (and Next.JS) is whether the browser has decided to jump to some point in history (the popstate event). It doesn't say whether it's from the past (e.g. the user pressed the browser "back" button) or the future, or perhaps the user long-pressed on the browser back button and jumped multiple pages back in history. This would screw up the history of paths.

  • Keeping track of history in the History API state, like this:

    history.pushState({ history: [...history.state.history ?? [], location.href] }, '/store/new-route');

    However, the only way to do this with Next.JS's router is to use query parameters and hide it with as={...}. However, in my testing, the hidden query parameters reset when the page reloads.

Changes

  • Removed padding around collection sliders

    Actually, this was implemented by keeping the padding around the item cards so their shadows don't get clipped, but adding negative margins to keep the first card aligned with the rest of the content. For the scrollbar on Chrome, equivalent padding was added to the scrollbar so it's aligned with the content too.

  • Added a border (same color as the shadow) around the item cards

  • Added a gap between the heading and mode switcher

  • The 404 page shows the logged in navbar, and store navbar under /store

  • Large point values are rendered as 10M

  • The view mode is stored in the URL so it doesn't reset when navigating between pages

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

Please include a screenshot of your Cypress testing suite passing successfully.

If you made any visual changes to the website, please include relevant screenshots below.

Before After
image image
image image
image image

I think that's all
Otherwise it be squishi
Pure CSS pagination!

However, for some reason scrollbars appear in the desktop help modal and idk why
You know it's bad when you start prefixing class names when you have a library that does that for you
The heading is similar on the leaderboard and storefront pages, and Trevor suggested making a component out of it. Here it is!

#42 (comment)
- Lined up first card with left edge of content without clipping card shadow
- Added border around card so a white image doesn't blend into the light theme background
- Store view mode in URL
- Add spacing in storefront subheading
@vercel
Copy link

vercel bot commented Oct 29, 2023

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 Nov 11, 2023 7:46pm

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.

looks great 🤩 2 tiny nits

Copy link
Contributor

@trevorkw7 trevorkw7 left a comment

Choose a reason for hiding this comment

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

Wow that commit history 😵‍💫 - thanks for documenting your whole journey with trying to come up with a better back button solution, I'm sure it'll be handy in the future. Really loving all the small changes to make the user experience in store across the board (especially love for the under-appreciated 404 page)! PR looks great 🥳

@SheepTester
Copy link
Member Author

A lot of the commits are actually from the previous PR; GitHub displays them because their histories are different (the storefront PR was merged with a squash commit)

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.

Storefront nitpicks
3 participants