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

fix: Use button for Ghost.io newsletter signup #337

Merged
merged 1 commit into from
Aug 9, 2024
Merged

fix: Use button for Ghost.io newsletter signup #337

merged 1 commit into from
Aug 9, 2024

Conversation

jpmcb
Copy link
Member

@jpmcb jpmcb commented Aug 8, 2024 •
edited by nickytonline
Loading

Description

  • Uses a button instead of the broken form for the newsletter.
  • Removes the duplicate maintaieners Newsletter.tsx component

Related Tickets & Documents

Closes: #336

Mobile & Desktop Screenshots/Recordings

Screenshot 2024-08-08 at 12 31 54 PM

Steps to QA

  1. Run landing page locally via npm run dev
  2. Go to https://deploy-preview-337--opensauced-landing.netlify.app/maintainers, scroll to bottom
  3. See "Subscribe" button

Tier (staff will fill in)

  • Tier 1
  • Tier 2
  • Tier 3
  • Tier 4

@jpmcb jpmcb requested a review from a team August 8, 2024 18:32
@netlify Netlify
Copy link

netlify bot commented Aug 8, 2024 •
edited
Loading

✅ Deploy Preview for opensauced-landing ready!

Name Link
🔨 Latest commit 5064b0a
🔍 Latest deploy log https://app.netlify.com/sites/opensauced-landing/deploys/66b52df8e1fd9200089b6beb
😎 Deploy Preview https://deploy-preview-337--opensauced-landing.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jpmcb
Copy link
Member Author

jpmcb commented Aug 8, 2024

cc @nickytonline - this seemed to have goofed up abunch of the spacing (and looks to have used 4 spaces) for the components I touched. npm run lint doesn't do anything and eslint --fix components/sections/home-page/Newsletter.tsx doesn't do anything either.

Thoughts?

nickytonline
nickytonline previously approved these changes Aug 8, 2024
View reviewed changes
Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Thanks @jpmcb! :shipit:

@jpmcb
Copy link
Member Author

jpmcb commented Aug 8, 2024

Any thoughts on fixing the spacing shenanigans I've created? ^ @open-sauced/engineering

@nickytonline nickytonline dismissed their stale review August 8, 2024 19:00

the button needs to be a link

nickytonline
nickytonline requested changes Aug 8, 2024
View reviewed changes
components/sections/home-page/Newsletter.tsx Outdated
Stay up to date with the latest OpenSauced news and trends.
</Typography>
<GradientBorderWrapper>
<button
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this being a button, make it a link (keep the styling) and set the href to https://news.opensauced.pizza/#/portal/signup. No need for an onClick

@nickytonline
Copy link
Member

Any thoughts on fixing the spacing shenanigans I've created? ^ @open-sauced/engineering

I formatted it on my local and pushed. Maybe it's some NeoVim wonkiness with this project in particular?

@nickytonline nickytonline force-pushed the newsletter-punt branch 2 times, most recently from fc4692e to 57d330a Compare August 8, 2024 19:14
@jpmcb jpmcb force-pushed the newsletter-punt branch from 57d330a to 5064b0a Compare August 8, 2024 20:43
@jpmcb
Copy link
Member Author

jpmcb commented Aug 8, 2024

I formatted it on my local and pushed. Maybe it's some NeoVim wonkiness

It's probably because eslintrc.json is more or less empty and you're relying on VScode internal tooling while I need to use the common tooling under the scripts in package.json to ensure conformity.

landing-page/.eslintrc.json

Lines 1 to 6 in 1bdcdd2

{
"extends": [
"next/core-web-vitals",
"prettier"
]
}

We can merge this through and I'll bring forward an .eslintrc.json that conforms with the app and api.

@jpmcb jpmcb requested review from nickytonline and a team August 8, 2024 20:46
@jpmcb jpmcb mentioned this pull request Aug 8, 2024
Merged
4 tasks
nickytonline
nickytonline approved these changes Aug 8, 2024
View reviewed changes
Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

:shipit:

@jpmcb jpmcb merged commit 4c029ca into main Aug 9, 2024
6 checks passed
@jpmcb jpmcb deleted the newsletter-punt branch August 9, 2024 16:55
@jpmcb jpmcb mentioned this pull request Aug 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickytonline nickytonline nickytonline approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Newsletter sign up no longer collects a user's email
2 participants
@jpmcb @nickytonline