Skip to content

DSD-1843: Hero primary refactor #1765

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 12, 2025
Merged

DSD-1843: Hero primary refactor #1765

merged 7 commits into from
May 12, 2025

Conversation

EdwinGuzman
Copy link
Member

@EdwinGuzman EdwinGuzman commented Mar 21, 2025 •
edited by jira bot
Loading

Fixes JIRA ticket DSD-1843

This PR does the following:

  • Updates the "primary" hero variant with updated grid system.

Open Questions

  • Not sure if this is what the refactor was meant to be so it's still WIP.

How has this been tested?

  • Storybook

Accessibility concerns or updates

  • N/A

Accessibility Checklist

  • Checked Storybook's "Accessibility" tab for color contrast and other issues.
  • The feature works with keyboard inputs including tabbing back and forward and pressing space, enter, arrow, and esc keys.
  • For hidden text or when aria-live is used, a screenreader was used to verify the text is read.
  • For features that involve UI updates and focusing on DOM refs, focus management was reviewed.
  • The feature works when the page is zoomed in to 200% and 400%.

Open Questions

Checklist:

  • I have updated the Storybook documentation and changelog accordingly.
  • I have added relevant accessibility documentation for this pull request.
  • All new and existing tests passed.

Front End Review:

  • Review the Vercel preview deployment once it is ready.

@vercel Vercel
Copy link

vercel bot commented Mar 21, 2025 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
nypl-design-system ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 8, 2025 2:02am

@EdwinGuzman EdwinGuzman added the In Progress Tickets or PRs that are being worked on. PRs marked as In Progress should not be merged. label Mar 21, 2025
@EdwinGuzman
Copy link
Member Author

@bigfishdesign13 updated to use the 12-col grid as in the POC. Let me know what you think and then I'll clean up the PR.

@EdwinGuzman EdwinGuzman marked this pull request as ready for review April 28, 2025 20:30
@EdwinGuzman EdwinGuzman added Needs Review Pull requests that are ready for peer review. and removed In Progress Tickets or PRs that are being worked on. PRs marked as In Progress should not be merged. labels Apr 28, 2025
bigfishdesign13
bigfishdesign13 requested changes May 6, 2025
View reviewed changes
Copy link
Collaborator

@bigfishdesign13 bigfishdesign13 left a comment

Choose a reason for hiding this comment

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

The grid looks good. It just needs some other small adjustments.

src/components/Hero/Hero.mdx Outdated
<ComponentDocsHeader
category="Basic Elements"
componentName="Hero"
summary="The `Hero` component is used to display a full width banner at the top of a page"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be simplified to A full-width banner at the top of a page.

src/components/Hero/Hero.mdx Outdated
Comment on lines 530 to 534
| Component Version | DS Version |
| ----------------- | ------------ |
| Added | `0.2.0` |
| Latest | `Prerelease` |

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed because it is in the revamped page header.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I saw it somewhere else as the updated v4 pattern so I moved it down here. I'll remove.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I recall that. I think that was a mistake on my part. 😨

src/theme/components/hero.ts Outdated
md: `". content content content content ."`,
},
gridTemplateColumns: "repeat(12, 1fr)",
gap: { base: "1.5rem", lg: "2rem" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to use the useResponsiveSpacing hook.

src/theme/components/hero.ts Outdated
},
content: {
bg: "ui.black",
color: getTextColor("body", "light", foregroundColor, isDarkText),
gridArea: "content",
gridColumn: { base: "1 / -1", md: "2 / 12", lg: "3 / 11" },
maxWidth: { md: "860px" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed. Yes, it allows the width of the content area to be greater than the 800px max width recommendation, but that's ok.

src/theme/components/hero.ts Outdated
Comment on lines 162 to 163
p: "l",
paddingInline: "1rem",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why both of these? If they are both needed, you can use s for paddingInline.

@bigfishdesign13 bigfishdesign13 added Changes Requested Pull requests where changes have been requested in peer review. Questions v4 Updates for the v4.0 feature branch and removed Needs Review Pull requests that are ready for peer review. labels May 6, 2025
@EdwinGuzman EdwinGuzman removed Changes Requested Pull requests where changes have been requested in peer review. Questions labels May 8, 2025
bigfishdesign13
bigfishdesign13 approved these changes May 9, 2025
View reviewed changes
Copy link
Collaborator

@bigfishdesign13 bigfishdesign13 left a comment

Choose a reason for hiding this comment

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

💯

@bigfishdesign13 bigfishdesign13 added the Ship It Pull requests that have been reviewed and approved. label May 9, 2025
@EdwinGuzman EdwinGuzman merged commit 614c965 into reservoir-v4 May 12, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bigfishdesign13 bigfishdesign13 bigfishdesign13 approved these changes

@oliviawongnyc oliviawongnyc Awaiting requested review from oliviawongnyc

@7emansell 7emansell Awaiting requested review from 7emansell

Assignees
No one assigned
Labels
Ship It Pull requests that have been reviewed and approved. v4 Updates for the v4.0 feature branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EdwinGuzman @bigfishdesign13