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

[Giving Day] New Heros #874

Merged
merged 17 commits into from
Mar 5, 2025
Merged

[Giving Day] New Heros #874

merged 17 commits into from
Mar 5, 2025

Conversation

cchrischen
Copy link
Collaborator

@cchrischen cchrischen commented Mar 3, 2025

Summary

Update heros to new design for giving day.

Notion/Figma Link

Figma

DESKTOP:

X Before After
Team Screenshot 2025-03-02 at 10 48 39 PM Screenshot 2025-03-02 at 10 48 11 PM
Products Screenshot 2025-03-02 at 10 48 50 PM Screenshot 2025-03-02 at 10 48 45 PM
Course Screenshot 2025-03-02 at 10 49 04 PM Screenshot 2025-03-02 at 10 49 12 PM
Initiatives Screenshot 2025-03-02 at 10 49 22 PM Screenshot 2025-03-02 at 10 49 16 PM
Sponsor Screenshot 2025-03-02 at 10 49 44 PM Screenshot 2025-03-02 at 10 49 33 PM
Apply Screenshot 2025-03-02 at 10 49 48 PM Screenshot 2025-03-02 at 10 53 25 PM

MOBILE

X Before After
Team Screenshot 2025-03-02 at 10 56 44 PM Screenshot 2025-03-02 at 10 56 35 PM
Products Screenshot 2025-03-02 at 10 57 14 PM Screenshot 2025-03-02 at 10 56 52 PM
Course Screenshot 2025-03-02 at 10 57 01 PM Screenshot 2025-03-02 at 10 57 31 PM
Initiaties Screenshot 2025-03-02 at 10 57 48 PM Screenshot 2025-03-02 at 10 57 42 PM
Sponsor Screenshot 2025-03-02 at 10 57 52 PM Screenshot 2025-03-02 at 10 58 00 PM
Apply Screenshot 2025-03-02 at 10 58 14 PM Screenshot 2025-03-02 at 10 58 09 PM

@dti-github-bot
Copy link
Member

dti-github-bot commented Mar 3, 2025

[diff-counting] Significant lines: 255.

</SectionWrapper>
</div>
</section>
<div className="lg:pb-24 pb-12">
Copy link
Collaborator

Choose a reason for hiding this comment

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

need some extra padding (tho less on mobile)


const InitiativePage = () => {
useTitle('Initiatives');
return (
<div className="bg-white flex flex-col">
<InitiativeHero />
<div className="lg:pb-24 pb-12 bg-[#000000]">
Copy link
Collaborator

@Bookie0 Bookie0 Mar 3, 2025

Choose a reason for hiding this comment

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

same thing as here, plus need to make the hero section black (rest of page is white)

@@ -17,25 +18,17 @@ export default function Page() {
useTitle('Products');

return (
<div className="overflow-x-hidden md:pt-[100px] xs:pt-9">
Copy link
Collaborator

Choose a reason for hiding this comment

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

removed padding as no longer needed


const { impacts } = impactData;
const { companies } = companyData;

const SponsorHero = () => (
<div
className="bg-black text-white md:my-[100px] xs:my-9 min-h-[calc(100vh-300px)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

also removed weird calc height stuff

</div>
</SectionWrapper>
<div className="relative"></div>
<div className="flex items-center pt-12 w-[100%]">
Copy link
Collaborator

Choose a reason for hiding this comment

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

padding changes to accomodate for banner

action?: {
buttonText: string;
link: string;
disabled?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

for the Apply button in hero section to be disabled when applications not open

className={`primary-button ${
action.disabled ? 'opacity-50 cursor-not-allowed pointer-events-none' : ''
}`}
href={action.disabled ? '' : action.link}
Copy link
Collaborator

Choose a reason for hiding this comment

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

empty href attribute (but still need it) so that it's still keyboard accessible even if disabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe we just don't render the button if the button is disabled. I think it's okay since the banner will indicate that the form is not open anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

i just read this which says we shouldn't be "disabling" links.

so i think ur right, let's just render a disabled <button> instead!

@cchrischen cchrischen marked this pull request as ready for review March 3, 2025 05:08
@cchrischen cchrischen requested a review from a team as a code owner March 3, 2025 05:08
Comment on lines +27 to +36
{action &&
(action.disabled ? (
<button className="primary-button opacity-50 cursor-not-allowed" disabled={true}>
{action.buttonText}
</button>
) : (
<a className="primary-button" href={action.link}>
{action.buttonText}
</a>
))}
Copy link
Collaborator

Choose a reason for hiding this comment

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

regarding this for future reference:

  • we shouldn't be disabling links (source)
  • we're instead rendering a disabled <button> component if the CTA action isn't meant to be interacted with. This <button> elem doesn't need to be keyboard focusable according to docs

Copy link
Collaborator

@Bookie0 Bookie0 left a comment

Choose a reason for hiding this comment

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

WOOHH let's ship it!! :shipit: :shipit: :shipit:

@cchrischen cchrischen merged commit e5f4d63 into main Mar 5, 2025
12 checks passed
@cchrischen cchrischen deleted the cc/new-heros branch March 5, 2025 20:52
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.

4 participants