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.

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

RENO-3681: NewsletterSignup Component #1422

Merged
merged 43 commits into from
Oct 12, 2023
Merged

RENO-3681: NewsletterSignup Component #1422

merged 43 commits into from
Oct 12, 2023

Conversation

aarnold101
Copy link
Contributor

@aarnold101 aarnold101 commented Sep 22, 2023 •
edited by isastettler
Loading

Newest branch Preview

@vercel
Copy link

vercel bot commented Sep 22, 2023 •
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 Oct 12, 2023 7:37pm

@github-actions
Copy link

Your pull request is missing a changelog—was that intentional?

EdwinGuzman
EdwinGuzman reviewed Sep 26, 2023
View reviewed changes
Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

This is great so far. We should chat about the focus feature for this component because the spec has changed. Not a problem but we should get it right.

src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.stories.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.test.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.test.tsx Outdated Show resolved Hide resolved
@nypl-wluisi nypl-wluisi changed the title RENO-3681/NewsletterSignup Component RENO-3681: NewsletterSignup Component Sep 26, 2023
oliviawongnyc
oliviawongnyc reviewed Sep 27, 2023
View reviewed changes
Copy link
Collaborator

@oliviawongnyc oliviawongnyc left a comment

Choose a reason for hiding this comment

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

Nice job!! This is a complicated component.

src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.stories.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.stories.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.mdx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.test.tsx Outdated Show resolved Hide resolved
src/helpers/getSectionColors.ts Outdated Show resolved Hide resolved
src/helpers/getSectionColors.ts Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Show resolved Hide resolved
bigfishdesign13
bigfishdesign13 requested changes Oct 11, 2023
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.

This looks very good. It just needs a few style changes, some language updates, and possibly one functionality adjustment.

src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/theme/components/newsletterSignup.ts Outdated Show resolved Hide resolved
src/theme/components/newsletterSignup.ts Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.tsx Outdated Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.mdx Show resolved Hide resolved
src/components/NewsletterSignup/NewsletterSignup.stories.tsx Show resolved Hide resolved
src/theme/components/newsletterSignup.ts Outdated Show resolved Hide resolved
src/theme/components/newsletterSignup.ts Outdated Show resolved Hide resolved
src/helpers/getSectionColors.ts Show resolved Hide resolved
@bigfishdesign13 bigfishdesign13 added the Changes Requested Pull requests where changes have been requested in peer review. label Oct 11, 2023
@EdwinGuzman
Copy link
Member

Most of the changes are from Marty and I just pointed out minor things. From what Marty pointed out:

  • Let's talk about using the Heading component inside vs outside the component. We currently have other components that set specific heading values so this might lead to a larger discussion. For now, I think the h3 fits slightly better than an h2 because it may be more versatile and perhaps used in more places than an h2. Unless if we already know that in DXP it will be used one level below the h1, then let's make it h3. @isastettler you mentioned you already tested this a bit in SCOUT so you might know best on its expected implementation and what level it should be.
  • Can we make the dark mode feature a new ticket and PR?

EdwinGuzman
EdwinGuzman approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

The latest update looks good to me! I think you have two or so unresolved comments from Marty.

@bigfishdesign13
Copy link
Collaborator

Everything looks great. Release the hounds!

@bigfishdesign13 bigfishdesign13 self-requested a review October 12, 2023 19:54
bigfishdesign13
bigfishdesign13 approved these changes Oct 12, 2023
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.

Yes!

@bigfishdesign13 bigfishdesign13 added Ship It Pull requests that have been reviewed and approved. and removed Changes Requested Pull requests where changes have been requested in peer review. labels Oct 12, 2023
@bigfishdesign13 bigfishdesign13 merged commit d0be2e0 into development Oct 12, 2023
4 checks passed
@EdwinGuzman EdwinGuzman deleted the RENO-3681/newslettersignup-component branch January 31, 2024 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isastettler isastettler isastettler left review comments

@oliviawongnyc oliviawongnyc oliviawongnyc left review comments

@EdwinGuzman EdwinGuzman EdwinGuzman approved these changes

@bigfishdesign13 bigfishdesign13 bigfishdesign13 approved these changes

@nypl-wluisi nypl-wluisi Awaiting requested review from nypl-wluisi

Assignees
No one assigned
Labels
Ship It Pull requests that have been reviewed and approved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aarnold101 @EdwinGuzman @bigfishdesign13 @oliviawongnyc @nypl-wluisi @isastettler