-
Notifications
You must be signed in to change notification settings - Fork 24
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
fix(hero): set a max width for header buttons when there are 2 buttons #2002
Conversation
✔️ Deploy Preview for dev-partners-bloom ready! 🔨 Explore the source changes: ebf1c88 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/6169d0ec8690140007b17a5b 😎 Browse the preview: https://deploy-preview-2002--dev-partners-bloom.netlify.app |
✔️ Deploy Preview for dev-bloom ready! 🔨 Explore the source changes: ebf1c88 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/6169d0ec48001b000871172d 😎 Browse the preview: https://deploy-preview-2002--dev-bloom.netlify.app |
✔️ Deploy Preview for dev-storybook-bloom ready! 🔨 Explore the source changes: ebf1c88 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/6169d0ec8690140007b17a5d 😎 Browse the preview: https://deploy-preview-2002--dev-storybook-bloom.netlify.app |
@@ -43,7 +43,7 @@ const Hero = (props: HeroProps) => { | |||
{props.buttonTitle && props.buttonLink && ( | |||
<> | |||
{props.secondaryButtonTitle && props.secondaryButtonLink ? ( | |||
<div className="grid md:grid-cols-6 gap-5 "> | |||
<div className="grid md:grid-cols-6 gap-5 max-w-screen-lg m-auto"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilyjablonski I just ended up making a super small change to try to make the button widths look a bit more balanced, but wasn't sure if there was a cleaner way to do it.
This just prevents the buttons from continuing to grow when the screen size goes above the large width.
@emilyjablonski not sure if you all have strong feelings about keeping font sizes pinned to the options available through tailwind. We'd ideally like our header to be 4rem instead of 3.5 rem, but the next size up in tailwind is |
@@ -37,13 +38,15 @@ const Hero = (props: HeroProps) => { | |||
} | |||
return ( | |||
<div className={`hero ${classNames}`} style={styles}> | |||
<h1 className="hero__title">{props.title}</h1> | |||
<h1 className={`hero__title ${props.extraLargeTitle ? "lg:text-6.5xl" : ""}`}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up adding an optional prop to make the title 4rem, open to feedback on it!
I'm not sure why tests are failing, it looks like a version issue? are they failing on dev too? |
@akegan are you rebased off of latest dev? that was an old build issue that should be resolved |
This PR stops the hero buttons from growing past the width they have at a large screen size re #1967
f5efded
to
ebf1c88
Compare
LGTM! The only small thing is that technically the style tag is reserved for |
oh! code style not site style. I'll change the PR title |
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pull Request Template
Issue
Addresses #1967
Description
This PR stops the hero buttons from growing past the width they have at a large screen size
Demo: https://user-images.githubusercontent.com/11825994/137377565-8531789a-47eb-41d2-8445-581037e4be9a.mp4
I did not end up changing the font size because tailwind jumps from 3.5rem to 4.5rem font sizes, and we're looking for 4rem.
Type of change
How Can This Be Tested/Reviewed?
?path=/story/headers-hero--with-secondary-button
at all screen sizes and verify that the new styles look OK.Checklist:
yarn generate:client
if I made backend changes