-
-
Notifications
You must be signed in to change notification settings - Fork 753
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: created newsroom section #749
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Hey @magicmatatjahu 👋, I need a quick review of yours in the below comments made by me. Since this PR contains a lot of changes, I request you to keep an eye on it I make any mistakes or wrongly implement something.
/au |
@akshatnema Great work! However found some bugs:
|
@akshatnema If you want, I can help you with articles and Twitter API. |
@akshatnema @magicmatatjahu Is there a way to fix the Netlify preview link so I can review the changes? |
@mcturco I recently resolved the commits from the GitHub code editor and hence not able to test it properly. I will soon update the PR to correct the errors. |
@mcturco You can now use the preview URL to preview the changes. |
Thanks @magicmatatjahu, if any help will be needed, I will sure ping you. @derberg @magicmatatjahu what is your idea to handle the latest news section? Should there be a |
Why not have it like https://github.com/asyncapi/website/blob/master/config/meetings.json |
@akshatnema As Łukasz wrote, it should be written in json. The element of article should have:
I know that in figma mock we have only title and published date, but we can extend it. |
@magicmatatjahu @derberg I understood your approach in making it like |
Signed-off-by: akshatnema <[email protected]>
Signed-off-by: akshatnema <[email protected]>
Signed-off-by: akshatnema <[email protected]>
Signed-off-by: akshatnema <[email protected]>
Signed-off-by: akshatnema <[email protected]>
Signed-off-by: akshatnema <[email protected]>
Signed-off-by: akshatnema <[email protected]>
This looks awesome @akshatnema 🎉 I think the only thing that I'm not a fan of is the Press section on mobile. Can we convert that to a vertical slider instead of the overflow scroll on mobile only? Or maybe we can only show 3 at a time and have a button that says "Load more" and then 3 more show up every time you press the button? Other than that small thing, I think it is good to go! |
Yeah, I can understand that it's not usual according to mobile view. But if we do such an implementation, we're making the page long in mobile view and you can see (in the above messages) that we have plans to add some press sections on the page as well. So, what's in my view is to leave it with the present implementation as right now. It isn't looking ugly at all from my perspective in mobile view 😄. But I certainly want views regarding this from you all? @derberg @magicmatatjahu |
It an so long that I would have already accepted it and fix errors in another PRs :) |
@magicmatatjahu Sounds good, let's 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.
LGTM! 🚀 @derberg Do you wanna add something before merge?
one small change in workflow is needed, left a comment to @akshatnema in a thread |
Signed-off-by: akshatnema <[email protected]>
@magicmatatjahu Sorry, you again have to review and approve the changes. |
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.
Amazing stuff @akshatnema 👏🏼
@derberg @magicmatatjahu also, as soon as you finished merging this PR, do take a look on PR #804. Since, we already fixed the Nodejs version error in v14.19.3, better let's shift the production to v14 and also come up with upgraded Tailwind versions so that we can use better classes and animations in newer version. And a suggestion, if possible, move the repo to node v16 so that any new contributors don't face any error with package-lock.json file versions. |
@akshatnema We need use v16 of nodejs because |
/rtm |
fyi workflow that updates lists works #889 I also can confirm it doesn't fail if something happens with scripts. As I ran it first, and there was error with token to YouTube, but error was just logged, and workflow run was green |
@derberg so even after the workflow gets into an error, it only logs the error and continues to make the PR with changes? |
@akshatnema no, it does not create a PR as there is no actual change, as there was API call failure. If there would be some new meetings, it would work, only videos would not be refreshed. Basically what I'm trying to say is that this workflow will never fail, will always be green even if tokens expire or stuff like that. We need to improve it, and I think the only solution is to use https://www.npmjs.com/package/@actions/core in anyway, works now, data is refreshed -> https://www.asyncapi.com/community/newsroom and I can only say: amazing work @akshatnema 👏🏼 🚀 |
Description☺️ .
This PR creates a newsroom section on the homepage of the website. Since various sections are being created, this PR will be in draft until the complete implementation of the new sections. Feedback//suggestions for the changes made in this PR are welcomed
Design
Figma file of the implemented design is here.
Related issue(s)
Resolves #562