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

Store item card and navbar components #59

Merged
merged 20 commits into from
Sep 1, 2023
Merged

Conversation

SheepTester
Copy link
Member

Info

Closes #53 and closes #54, which are blockers for most of the store page issues.

Description

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

I added a store item card component and a store navbar component.

Changes

  • Added an ItemCard component which takes an image URL, title, and cost. Presumably, these cards will link you to the item page, so there's also an optional href prop. I noticed that the designs had a hidden "Out of stock" message, so I also added an optional inStock prop which defaults to true that, when set to false, will show "Out of stock" next to the price.

    <ItemCard
      image="https://acmucsd.s3.us-west-1.amazonaws.com/portal/profiles/07789a16-8326-4edc-ad2d-fc6193cd1ee3.jpg"
      cost={42069}
      title="Nishant (not for sale)"
      href="/leaderboard"
      inStock={false}
    />
    • The Figma uses rgba(228, 228, 228, 0.5) as the card's shadow color, but this looks like a very unpleasant light grey shadow in dark mode. However, in themes.scss, --theme-accent-line-2-transparent is still the same light grey shadow. I could instead use --theme-accent-line-1-transparent, which looks fine in dark mode, but in light mode the shadow is very jarring. I added a CSS variable --theme-shadow that uses a light grey shadow in light mode and the black shadow in dark mode.
  • Added a Navbar component which takes the user's balance and optionally, backUrl. If there is no page to go back to, then leaving backUrl unspecified will make the back button go away.

    Ideally, Navbar would be able to get the user's balance by itself, but I don't know if it can access the user: PrivateProfile on its own.

    <Navbar balance={user.credits} backUrl="/store" />
  • I also added a helper component Diamonds which renders the number of diamonds with a comma (using the Internationalization API) and includes the little blue diamond. The diamond icon has a hidden "diamond" text inside it so screenreaders can read it out loud.

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. no

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

I put the components on the storefront page just to demo them; the storefront page isn't done yet

Dark mode, desktop:

Dark mode, desktop

Light mode, mobile:

Light mode, mobile

@vercel
Copy link

vercel bot commented Aug 2, 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 Sep 1, 2023 3:43am

@SheepTester SheepTester changed the title Store item and Store item card and navbar Aug 2, 2023
@SheepTester SheepTester changed the title Store item card and navbar Store item card and navbar components Aug 2, 2023
Copy link
Contributor

@raymosun raymosun left a comment

Choose a reason for hiding this comment

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

Hey, nice job on this. I really like the implementation of Diamonds and the props setup for the other components 🤑

  • I think it's fine to pass in the credits from the page. Accessing the private profile again in the navbar would probably require an additional API call.

  • One other thing is can you look into modifying the history with the back button instead of just using a link. Currently, clicking the back button will push another page to your history so pushing the custom back button and then pushing the browser back button takes you "forward" to the page you were on. It might be okay to just use history.back() have the back button mirror the browser back button, with maybe some consideration for not "backing" out of the store.

src/components/store/Diamonds/index.tsx Outdated Show resolved Hide resolved
src/components/store/Navbar/index.tsx Outdated Show resolved Hide resolved
src/components/store/Navbar/index.tsx Outdated Show resolved Hide resolved
@SheepTester
Copy link
Member Author

SheepTester commented Aug 11, 2023

I added an optional description prop to ItemCard because I think the store item card and storefront collection card look very similar, and I don't want to have repetitive code.

I also addressed @raymosun's feedback:

  1. The back button uses history.back() now (well, the one provided by next/router).

    Currently, it's up to each page whether to show the back button. I wanted to try reading history from next/router, but it doesn't look like they provide it. Otherwise, I'd only show the back button if the previous page is under /store but isn't the storefront page.

    If someone linked directly to an item page, the back button would take the visitor out of the website rather than to the storefront page, which might be a bit jarring, but I'm not sure what user expectations are for a back button when you're linked directly to a sub page.

  2. I just pushed "diamonds" out of the square with a bit of padding rather than changing the highlight color.

  3. The back icon is in asserts/icons/back-chevron.svg now. I was considering replacing back-arrow.svg, which is larger and has a shaft and isn't used anywhere, but I decided to leave it be.

  4. The cart links were moved to config.ts.

@raymosun
Copy link
Contributor

Hey Sean, thanks for addressing the changes! I think prepping for the component to handle item card and collection card is a good idea.

So, the only thing that's still iffy is the back button, but I think it's good enough for now so we can go ahead and merge so the other store pages aren't blocked and maybe address it with a later issue.

@SheepTester
Copy link
Member Author

image
bruh

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.

Components look good so far! Some questions and comments and a few nits to address.

src/components/store/ItemCard/index.tsx Outdated Show resolved Hide resolved
src/components/store/Navbar/index.tsx Outdated Show resolved Hide resolved
public/assets/icons/back-chevron.svg Outdated Show resolved Hide resolved
src/components/store/Diamonds/index.tsx Outdated Show resolved Hide resolved
src/components/store/ItemCard/style.module.scss Outdated Show resolved Hide resolved
src/components/store/Navbar/index.tsx Outdated Show resolved Hide resolved
src/components/store/Navbar/style.module.scss Outdated Show resolved Hide resolved
src/components/store/index.ts Outdated Show resolved Hide resolved
src/pages/store/index.tsx Outdated Show resolved Hide resolved
src/pages/store/index.tsx Outdated Show resolved Hide resolved
@farisashai
Copy link
Member

Can you include edge case screenshots of handling an item card with a description too long?

@farisashai
Copy link
Member

image

fitting content is a little funky on iPhone SE size, up to you if you want to address it. nobody is going to have 10 million points anytime soon 😮

@SheepTester
Copy link
Member Author

Can you include edge case screenshots of handling an item card with a description too long?

image

When one card has an insanely long description, the text wraps and the other cards' height match the height of the longest card. I believe this is consistent with the Figma (below), and personally I think it's a bit better than having staggered card heights. However, if having a lot of blank space on shorter cards isn't desirable, then changing it is a single-line change (align-items: flex-start)

image

@SheepTester SheepTester mentioned this pull request Aug 18, 2023
15 tasks
SheepTester added a commit that referenced this pull request Aug 18, 2023
`picture` can be null; I decided to update the type instead of blaming the backend data

I also added `screen and` to media queries and updated some rem values per feedback in #59
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.

the store nav back button needs to function more robustly in any possible scenario and the cards shouldn't break the height that much with long text content but i'll approve and leave those to you to fix later

@SheepTester SheepTester merged commit ae9d67b into main Sep 1, 2023
2 of 3 checks passed
@SheepTester SheepTester deleted the sean/store-item-card branch September 1, 2023 03:54
SheepTester added a commit that referenced this pull request Oct 15, 2023
* Create store item card component

* Add card background and "out of stock" support

* Turn card into link; allow image to be squished on mobile

* Card hover effect

I think that's all

* Create navbar

* Commit the correct index.tsx

* Make "xyz diamonds" its own component

* Back icon; screen-reader accessible diamond icon

* Load balance from user profile

* Make the shadow less harsh in light mode

* Shrink navbar on mobile, per Figma

Otherwise it be squishi

* Storefront hero content

* Attempt making a l o n g rainbow

* A bit of mobile support for the hero

* Move Hero to its own component

* Help modal

* Mobile help modal

Pure CSS pagination!

However, for some reason scrollbars appear in the desktop help modal and idk why

* ayo?? gradient text outline in CSS is possible??

* Add description prop to item cards so they can be used for storefront collection cards as well

* Move back icon into assets/, navbar URLs to config

* Change back link to `history.back()` button

* Make rainbow stretch across page

* Try making the rainbow fit mobile

* Move rainbow to its own component

You know it's bad when you start prefixing class names when you have a library that does that for you

* The tilted rainbow looks fine on mobile now 🎉

* Get collections from store API

* Create Title component

The heading is similar on the leaderboard and storefront pages, and Trevor suggested making a component out of it. Here it is!

#42 (comment)

* Finish storefront collections view

* Browse all items view

* Clean up files

* Adjust heading sizes

They somewhat match the Figma, but not really. Maybe moving the heading to a separate component was a bad idea if the font sizes are going to be different every time

* Hero animations and interactive decorations 🎥😎

* Require that an item card be EITHER a store item or a collection, per faris feedback

Based on SignInButton

* Address Faris' feedback

* Show costs under items

* Add Imgur etc to image host allowlist (dev only)

`picture` can be null; I decided to update the type instead of blaming the backend data

I also added `screen and` to media queries and updated some rem values per feedback in #59

* Remove <Title> because each use required overriding the font size

* Image load animation

So when it loads, it's not as jarring when it appears

* Remove redundant useEffect

---------

Co-authored-by: Faris Ashai <[email protected]>
This was referenced Oct 28, 2023
SheepTester added a commit that referenced this pull request Nov 11, 2023
* Create store item card component

* Add card background and "out of stock" support

* Turn card into link; allow image to be squished on mobile

* Card hover effect

I think that's all

* Create navbar

* Commit the correct index.tsx

* Make "xyz diamonds" its own component

* Back icon; screen-reader accessible diamond icon

* Load balance from user profile

* Make the shadow less harsh in light mode

* Shrink navbar on mobile, per Figma

Otherwise it be squishi

* Storefront hero content

* Attempt making a l o n g rainbow

* A bit of mobile support for the hero

* Move Hero to its own component

* Help modal

* Mobile help modal

Pure CSS pagination!

However, for some reason scrollbars appear in the desktop help modal and idk why

* ayo?? gradient text outline in CSS is possible??

* Add description prop to item cards so they can be used for storefront collection cards as well

* Move back icon into assets/, navbar URLs to config

* Change back link to `history.back()` button

* Make rainbow stretch across page

* Try making the rainbow fit mobile

* Move rainbow to its own component

You know it's bad when you start prefixing class names when you have a library that does that for you

* The tilted rainbow looks fine on mobile now 🎉

* Get collections from store API

* Create Title component

The heading is similar on the leaderboard and storefront pages, and Trevor suggested making a component out of it. Here it is!

#42 (comment)

* Finish storefront collections view

* Browse all items view

* Clean up files

* Adjust heading sizes

They somewhat match the Figma, but not really. Maybe moving the heading to a separate component was a bad idea if the font sizes are going to be different every time

* Hero animations and interactive decorations 🎥😎

* Require that an item card be EITHER a store item or a collection, per faris feedback

Based on SignInButton

* Address Faris' feedback

* Show costs under items

* Add Imgur etc to image host allowlist (dev only)

`picture` can be null; I decided to update the type instead of blaming the backend data

I also added `screen and` to media queries and updated some rem values per feedback in #59

* Remove <Title> because each use required overriding the font size

* Image load animation

So when it loads, it's not as jarring when it appears

* Remove redundant useEffect

* Various nitpicks + store view mode in URL

- 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

* Improve accessibility in navbar

* Show store and signed-in navbar on 404 page

* Use compact number format for numbers >= 1M

* Fix height of 404 page under /store

* Decrease minimum for compact diamond format

Co-authored-by: Faris Ashai <[email protected]>

* Fix typo on 404

Co-authored-by: Faris Ashai <[email protected]>

---------

Co-authored-by: Faris Ashai <[email protected]>
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.

Implement Store Navbar Create Store Item Card
3 participants