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

107 enhancement secondary partners layout updates #113

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ogorman89
Copy link
Contributor

This PR:

Resolves #107

Changes for desktop version of SecondaryPartners component only

  • Changed the grid layout to something closer to 60-40 (text-box to logo-box)
  • Cards still appear to be scaling correctly based on the largest card (ORDSN)
  • Fixed a small issue that we rendering an empty <span> if there was no testimonialAuthor prop
    • Changed the other conditional renders to use the && short-circuit convention for consistency
  • Design changes (based on my own opinion):
    • Changed some of the padding and alignment of the SecondaryPartners component
    • Centered the logo and text vertically; I think this makes the cards look better, but I can easily revert this if you guys like it better with some offset

Screenshots (if applicable):

Here's what it looks like with these changes
Screen Shot 2024-02-02 at 3 55 57 PM


Future Steps/PRs Needed to Finish This Work (optional):

N/A

@ogorman89 ogorman89 added the enhancement New feature or request label Feb 3, 2024
@ogorman89 ogorman89 self-assigned this Feb 3, 2024
@ogorman89 ogorman89 linked an issue Feb 3, 2024 that may be closed by this pull request
Copy link

vercel bot commented Feb 3, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
codepdx-website ⬜️ Ignored (Inspect) Visit Preview Feb 7, 2024 1:05am

Copy link
Contributor

@Jared-Krajewski Jared-Krajewski left a comment

Choose a reason for hiding this comment

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

request change for minor padding in comment . I am sorta partial to the logo being staggered, to me the white space within the card seems odd with it centered.. but If you want to change it I wont stop you either.

<Grid item pl={'6%'} {...partnerGridStyle} order={contentOrder}>
<Typography variant="body1" p={'5% 15% 3% 0'}>
<Grid item {...partnerGridStyle} md={7} order={contentOrder}>
<Typography variant="body1" p={'5% 10% 3% 10%'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

i think padding here can be 0% 10% 3% 10%, also the pt: '20px' can go.

const partnerGridStyle = {
  xs: 12,
  md: 6,
  display: 'flex',
  justifyContent: 'center',
  alignItems: 'center',
  flexDirection: 'column',
  pt: '20px' <----
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jared-Krajewski

I pushed a commit to adjust the padding & alignment. I aligned items top, to be more like the figma file.

The CETI card looks a little off to me but not terrible there was no CETI in the initial figma mockup for comparison.

Note, I moved the 5% padding to the parent grid element. If I do the same padding on both child elements, they don't come out exactly equal. 🥳 I think this is fixable, but it seemed silly to waste too much time chasing it if padding the parent element works to make them the same size.

Screen Shot 2024-02-06 at 5 04 16 PM

@andycwilliams
Copy link
Member

andycwilliams commented May 2, 2024

@AJSterner did you intend to close this? I don't think this issue has been resolved.

Edit: Seems like this was closed by mistake. Reopening.

@andycwilliams andycwilliams reopened this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Secondary Partners Layout Updates
3 participants