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

New design for incentive cards #169

Merged
merged 3 commits into from
Jun 24, 2024
Merged

New design for incentive cards #169

merged 3 commits into from
Jun 24, 2024

Conversation

oyamauchi
Copy link
Member

@oyamauchi oyamauchi commented Jun 21, 2024

Description

I believe this is exactly as designed. There are a couple of things
that weren't 100% clear to me:

  • The max-width of these cards is 720px; does that mean the max width
    of the entire calculator should also be 720px, including the form?
    (That's what I've implemented here.)

  • If the headline is long, it can run into the top-right CTA link
    (especially if the CTA text is "Learn how to apply"). The designs
    didn't specify what should happen in that case. I made the headline
    line-wrap, while the CTA link stays flush against the top and right.
    The CTA link never line-wraps. There's at least a 4-rem (16px) gap
    between the headline and CTA. Screenshot of wrapped and unwrapped
    cases attached.

Screenshot 2024-06-20 at 9 44 24 PM

Follow-up

CI will fail because of translations. If copy looks good, do those.

Test Plan

Look at the cards on desktop (whole variety of window sizes) and
mobile (portrait and landscape). Make sure to look at an IRA rebate
card, which gets two chips that line-wrap on narrow layout.

## Description

I believe this is exactly as designed. There are a couple of things
that weren't 100% clear to me:

- The max-width of these cards is 720px; does that mean the max width
  of the _entire calculator_ should also be 720px, including the form?
  (That's what I've implemented here.)

- If the headline is long, it can run into the top-right CTA link
  (especially if the CTA text is "Learn how to apply"). The designs
  didn't specify what should happen in that case. I made the headline
  line-wrap, while the CTA link stays flush against the top and right.
  The CTA link never line-wraps.  There's at least a 4-rem (16px) gap
  between the headline and CTA. Screenshot attached.

## Follow-up

CI will fail because of translations. If copy looks good, do those.

## Test Plan

Look at the cards on desktop (whole variety of window sizes) and
mobile (portrait and landscape). Make sure to look at an IRA rebate
card, which gets two chips that line-wrap on narrow layout.
Copy link

vercel bot commented Jun 21, 2024

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

Name Status Preview Updated (UTC)
embed-rewiringamerica-org ✅ Ready (Inspect) Visit Preview Jun 21, 2024 7:55pm

@@ -27,8 +31,9 @@ export const Card = forwardRef<
'grid-cols-1',
'gap-4',
isFlat && 'mx-auto text-center max-w-78',
isFlat && 'px-4 py-8',
!isFlat && 'p-4 sm:p-6',
padding === 'large' && 'px-4 py-8',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should we add these as custom styles and make the className dynamic instead? (wondering if this padding css could be made reusable).

@@ -72,5 +72,11 @@
margin: 0 auto; /* center on page */
display: block; /* required for max-width to kick in */
width: 100%;
max-width: 1280px;
max-width: 720px;
Copy link
Contributor

Choose a reason for hiding this comment

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

The max-width of these cards is 720px; does that mean the max width
of the entire calculator should also be 720px, including the form?
(That's what I've implemented here.)

So, I was curious about this as well. The Calc and c
ards don't look as good on really large screens with this set to only 720 px and I could have been looking at the wrong place in the designs, but it looked like at least here...
Screenshot 2024-06-21 at 9 19 32 AM

That the cards should be allowed to be larger.

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed in DMs with Emiola that the whole thing is actually supposed to be 720px max-width. (I think you're looking at an outdated part of the Figma -- the part I'm working from has gotten rid of the pill button project selector.)

I'm going to update the demo page to match that max width.

@oyamauchi
Copy link
Member Author

Discussed with Emiola in DMs -- the treatment of the headline is basically right, though we may just tweak its line height.

I'll see about translations now.

@oyamauchi oyamauchi merged commit a500c5c into main Jun 24, 2024
3 checks passed
@oyamauchi oyamauchi deleted the owen.new-card branch June 24, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants