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

feat: add new newsletter view #644

Merged
merged 5 commits into from
Apr 5, 2022

Conversation

derberg
Copy link
Member

@derberg derberg commented Mar 31, 2022

This PR introduces asyncapi.com/newsletter page to make it easy to link to a location where people can subscribe to Newsletter.

We had this page in the past but it was lost during change from Hugo to Next.js

Related issue(s)
See also #33. The See also #33` option will not automatically close the issue after the PR merge. -->

@netlify
Copy link

netlify bot commented Mar 31, 2022

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e692f03
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/624c2a69b6a1700008b9b9ad
😎 Deploy Preview https://deploy-preview-644--asyncapi-website.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 settings.

@magicmatatjahu
Copy link
Member

Ok for me, but I think we should change the design of this page because we are using the same colors for newsletter as is on the home page. @mcturco Could you check https://deploy-preview-644--asyncapi-website.netlify.app/newsletter and suggest changes, or should we leave it as it is?

@mcturco
Copy link
Member

mcturco commented Apr 4, 2022

Hmm yeah I agree with @magicmatatjahu here. I know the idea was to be a quick solution, but it looks a little bit too "copy and paste" for me, especially because there are no other sections on this new page to break up the dark background.

Instead, I would suggest that we remove the dark background (keep it white), change the Heading & Paragraph components to use their default textColor and then just make sure that the height of the section is at least 90vh so that it doesn't show white space below the footer on taller screens.

@derberg
Copy link
Member Author

derberg commented Apr 5, 2022

event though #designismypassion this time I will agree with you 😆

but it looks a little bit too "copy and paste" for me

who said it wasn't 🤣

I literally had to google what vh is and what you want me to do 😄

Let me know what you think. When I did some tests on my different screens, vh (viewport-height 😉 ) is better set to 80 rather than 90

tbh, I can also remove the footer entirely, I think we can agree that on such a specific view, we do not need it 😄

@magicmatatjahu
Copy link
Member

@magicmatatjahu
Copy link
Member

@derberg Ok I see that documentation pages are broken, I will check this.

@derberg
Copy link
Member Author

derberg commented Apr 5, 2022

@magicmatatjahu sometimes you are so kind ❤️
@mcturco how does it look like now?

@magicmatatjahu
Copy link
Member

you're welcome :) but still some views don't work, for example sticky navbar only works up to a certain height. I need to fix some global styles to make everything work as it should, give me some time.

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM! But please wait for @mcturco feedback.

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

LGTM too 🚢 🇮🇹

@mcturco
Copy link
Member

mcturco commented Apr 5, 2022

@mcturco how does it look like now?

Yesss looks way better, thank you for making those changes!

Also side note - and we don't have to address this in this PR: Do all the forms on the website do this after submit? (See screenshot below) I think it would be better if we kept the submission notification on the same screen without redirecting to a netlify page. I could submit a separate issue for this

Screen Shot 2022-04-05 at 9 49 08 AM

@derberg
Copy link
Member Author

derberg commented Apr 5, 2022

@mcturco definitely need to be discussed on separate issue, not sure if it can be changed, Netlify Forms are used for it

@derberg
Copy link
Member Author

derberg commented Apr 5, 2022

/rtm

@asyncapi-bot
Copy link
Contributor

Hello, @derberg! 👋🏼
This PR is not up to date with the base branch and can't be merged.
You can add comment to this PR with text: /autoupdate or /au. This way you ask our bot to perform regular updates for you. The only requirement for this to work is to enable Allow edits from maintainers option in your PR. Otherwise the only option that you have is to manually update your branch with latest version of the base branch.
Thanks 😄

@asyncapi-bot asyncapi-bot merged commit f719b47 into asyncapi:master Apr 5, 2022
@derberg
Copy link
Member Author

derberg commented Apr 5, 2022

@KhudaDad414 something ain't right, have a look at #644 (comment). It was for sure up to date, and ready to merge

@mcturco
Copy link
Member

mcturco commented Apr 5, 2022

@derberg yeah this was happening to me a lot yesterday and was wondering why

@KhudaDad414
Copy link
Member

@derberg on it. please keep this branch https://github.com/derberg/website-asyncapi/tree/add-newslinkpage for now so I can do some testing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants