-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Feat: Add the Newsletter feature to the website. #308
Conversation
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.
Thanks for this PR @ayushnau !
We'll need to do some additional work to make sure we provide the name and email fields alongside with the subscribe button in the landing page like the example provided from AsyncAPI.
In addition, the new page should have all the styling than the other pages of the website including headers and footer... you can see a similar page in "https://json-schema.org/implementations" and the equivalent page in ASyncAPI site:
https://www.asyncapi.com/newsletter
Is it ok to rename the page subscribe to newsletter?
Could it be possible to avoid uppercase in the name of Newsletter.tsx?
@benjagm can you elaborate the newsletter point. do you want it for filename or for component name . cause i can see in some places we have used uppercases in filename and also in component name. |
Forget about my comment on this one @ayushnau. My bad, this as internal component and uppercase in the first letter is irrelevant. |
ok can you review the pr. here is how it looks. @benjagm |
Also please provide me the api for subscribe. will integrate it. |
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.
Great progress @ayushnau !!! Thanks a lot.
Just few comments: The screenshot I provided was just an example, so it is not necessary to add that black background.
-
For the newsletter page: Background should be white, fonts smaller and increase the horizontal padding to at least 8.
-
For showing the component in the landing page remember it is going to be shown in this section (make sure the styles work here and also in the newsletter page):
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.
You did a lot of progress @ayushnau , Thanks!!
I left some additional change requests to make sure we are covering all the scenarios.
Great progress so far!!! Congrats. We are very close.
components/Newsletter.tsx
Outdated
wrapperClassName?: string | ||
className?: string | ||
} | ||
const NewsletterForm: React.FC<NewsletterFormProps> = ({ wrapperClassName = '', className = '' }) => { |
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.
This component is going to be presented in two scenarios:
- In the newsletter page with while background and black letters. (default)
- In the landing page call to action banners, with blue background and white letters. (see image)
With that in mind you will need to define an input parameter allowing to define what of those 2 behaviours the component will render.
pages/index.page.tsx
Outdated
@@ -320,7 +320,7 @@ const Home = (props: any) => { | |||
<section className='w-full h-[300px] lg:h-[367px] bg-gradient-to-r from-primary from-1.95% to-endBlue clip-both'> | |||
<div className='lg:w-full mx-auto text-center mt-28 '> | |||
<h2 className='text-h3mobile lg:text-h3 text-white mb-6'>Start contributing to JSON Schema</h2> | |||
<button className='w-[170px] h-[45px] mx-auto rounded border-2 bg-primary text-white font-semibold'><a href='https://github.com/json-schema-org#-contributing-to-json-schema'>Contribute</a></button> | |||
<button className='w-[170px] h-[45px] mx-auto rounded border-2 bg-primary text-white font-semibold'><a href='/newsletter'>Subscribe</a></button> |
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.
Wha we need here is to show the component itself, no a button to navigate to the newsletter page. We need here to remove that extra step. Make it is easy for the user to subscribe.
See my first comment regarding how to make the component use different fonts/colours depending on the context.
My expectation is to implement the same approach than asyncapi. They show component in the landing page with white fonts:
https://www.asyncapi.com/en
And they show the component with black fonts in the newsletter page:
https://www.asyncapi.com/newsletter
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.
added.
Pasted ss for newsletter page and landing page both in mobile and web views. |
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.
@AyushNautiyalDeveloper @ayushnau Are you the same person?
Please check and increase the horizontal paddings or margins in mobile to make it look similar to AsyncAPI:
For desktop: This is what I see
I think something is missing when appearing in the landing page. In addition in this case I think the background of the inputboxes will look better white.
made the changes. here is the images for reference. @benjagm |
and yes we both are same person. |
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.
Excelent work @ayushnau!
Just pushed some extra code to make it work with our mailchimp account. Will merge it in the next days!
ok sure that would be great. @benjagm |
@ayushnau Do you mind rebasing and solving the merge conflicts? We'll merge it. |
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.
Can we please avoid the changes in package.json and yarn.lock? I think all those changes are not necessary.
Can you please change the target branch to https://github.com/json-schema-org/website/tree/web-release-3? |
@ayushnau do you need any help for the pr, feel free to ask : ) |
@ayushnau its almost done let me know if you need any help. |
Can you please change the target branch to https://github.com/json-schema-org/website/tree/web-release-3? |
Are we closing this in favour of #489 ? |
yes. |
Closed in favour of #489 |
GitHub Issue: #243
Summary:
Do you think resolving this issue might require an Architectural Decision Record (ADR)? (significant or noteworthy)
No
No