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: migrate layout and campaigns #2743

Merged
merged 23 commits into from
Mar 28, 2024

Conversation

Shurtu-gal
Copy link
Contributor

No description provided.

Copy link

netlify bot commented Mar 6, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 323c440
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/66046f308581680009ad2edd
😎 Deploy Preview https://deploy-preview-2743--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 configuration.

package.json Outdated Show resolved Hide resolved
.eslintrc Outdated
@@ -26,8 +26,7 @@
"error",
{
"code": 120,
"ignoreUrls": true,
"ignorePattern": "*className=([\\s\\S]*?)*" // Ignore classnames
Copy link
Member

Choose a reason for hiding this comment

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

We should have this line because there are tailwind classNames inside components, where we don't want to create an error if the maxlen rule is not followed.

Copy link
Member

Choose a reason for hiding this comment

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

IMO we should disable this max-len rule. I disabled it in my PR since some classNames were greater than 120 characters but there was still an error.

Though, is there any example case where max-len rule would actually help us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was facing a lot of problems too. Hence I had added this in my previous PR. However as eslint was still throwing errors, I removed it.

Sth can be done about max-len though as tailwind classNames get really big.

components/Head.tsx Show resolved Hide resolved
components/Head.tsx Outdated Show resolved Hide resolved
lib/api.ts Outdated Show resolved Hide resolved
components/layout/Layout.tsx Outdated Show resolved Hide resolved
components/layout/GenericLayout.tsx Outdated Show resolved Hide resolved
@Shurtu-gal
Copy link
Contributor Author

@akshatnema the navigation components are being done by @vishvamsinh28 so DocsLayout would have to wait until then.

@vishvamsinh28
Copy link
Contributor

@akshatnema @Shurtu-gal will open pr for navigation by tomorrow night

Comment on lines 1 to 2
/* eslint-disable no-unused-vars */
/* eslint-disable unused-imports/no-unused-vars */
Copy link
Member

Choose a reason for hiding this comment

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

Why these rules are disabled?

Copy link
Member

Choose a reason for hiding this comment

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

campaigns are not part of this migration PR. Why are we making these component files in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because they are being used by layouts.

Copy link
Member

Choose a reason for hiding this comment

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

Comment those particular lines where they are about to get used.

Copy link
Member

Choose a reason for hiding this comment

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

Kindly remove the unnecessary changes in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

pages/_app.tsx Outdated
</div>
<AppContext.Provider value={{ path: router.asPath }}>
<Head>
<script async defer src='https://buttons.github.io/buttons.js'></script>
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding this script tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this was there before too, in the master branch.

return <GenericPostLayout post={post as unknown as IPosts['blog'][number]}>{children}</GenericPostLayout>;
}

return children as JSX.Element;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return children as JSX.Element;
return children as React.ReactNode;


return (
<GenericPostContext.Provider value={{ post }}>
<AnnouncementHero className='m-4 text-center' small={true} />
Copy link
Member

Choose a reason for hiding this comment

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

You can comment out this line and better exclude the campaigns components from this PR.

Comment on lines 1 to 2
/* eslint-disable unused-imports/no-unused-vars */
/* eslint-disable no-unused-vars */
Copy link
Member

Choose a reason for hiding this comment

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

Why are we disabling these rules for whole file?


/**
* @description The banner to use for Announcement
* @param {String} props.title - The title of the banner
Copy link
Member

Choose a reason for hiding this comment

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

Use string here. Applicable on all files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 20 to 24
* @param className - The class name of the TOC
* @param cssBreakingPoint - The CSS breaking point of the TOC
* @param toc - The table of contents
* @param contentSelector - The content selector
* @param depth - The depth of the TOC
Copy link
Member

Choose a reason for hiding this comment

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

Specify the types of params here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 17 to 21
* @param {String} props.title - The title of the page
* @param {String} props.description - The description of the page
* @param {String} props.image - The image of the page
* @param {String} props.rssTitle - The RSS title of the page
* @param {String} props.rssLink - The RSS link of the page
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {String} props.title - The title of the page
* @param {String} props.description - The description of the page
* @param {String} props.image - The image of the page
* @param {String} props.rssTitle - The RSS title of the page
* @param {String} props.rssLink - The RSS link of the page
* @param {string} props.title - The title of the page
* @param {string} props.description - The description of the page
* @param {string} props.image - The image of the page
* @param {string} props.rssTitle - The RSS title of the page
* @param {string} props.rssLink - The RSS link of the page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 23 to 24
* @param props.post - The blog post to render in the layout
* @param props.children - The content of the blog post
Copy link
Member

Choose a reason for hiding this comment

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

Specify the type of params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@akshatnema akshatnema changed the title feat: migrate layout feat: migrate layout and campaigns Mar 28, 2024
@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit ce3446d into asyncapi:migrate-ts Mar 28, 2024
18 checks passed
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.

5 participants