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 navigation #2765

Merged
merged 69 commits into from
Mar 29, 2024

Conversation

vishvamsinh28
Copy link
Contributor

migrated navigation

Copy link

netlify bot commented Mar 12, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit cfb4c8c
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/6605a56886c8c600086d74c8
😎 Deploy Preview https://deploy-preview-2765--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.


/**
* @description Renders a logo that is clickable and redirects to a specified URL.
* @param {ClickableLogoProps} props - The props for the component.
Copy link
Member

@anshgoyalevil anshgoyalevil Mar 14, 2024

Choose a reason for hiding this comment

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

Define all params in jsdoc instead of the whole interface

Copy link
Member

Choose a reason for hiding this comment

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

Do this for all components.

Comment on lines 18 to 19
<Link href={href}>
<a className={className}>
Copy link
Member

Choose a reason for hiding this comment

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

Add the legacyBehavior prop to Link component or merge the anchor to Link. https://nextjs.org/docs/messages/invalid-new-link-with-extra-anchor

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 using legacyBehavior, replace the a with span tag.

link: string;
className: string;
borderClassName: string;
Icon: React.ComponentType<any>;
Copy link
Member

Choose a reason for hiding this comment

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

Why this key is duplicated when there already exists an icon key with the same value?

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 using React.ComponentType<any>, you can define a IconType in types folder as React.ReactElement.

className?: string;
onChange?: (selected: string) => void;
options: Option[];
selected?: string;
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
selected?: string;
selected: string;

@vishvamsinh28
Copy link
Contributor Author

@akshatnema updated the navigation pr

Screenshot 2024-03-25 at 3 20 32 PM Screenshot 2024-03-25 at 3 20 41 PM

onChange = () => {},
options = [],
selected
}: SelectProps) {
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 defining LanguageSelect props with SelectProps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because they both have same props

Comment on lines 9 to 11
post: {
slug: string;
};
Copy link
Member

Choose a reason for hiding this comment

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

Define post using Post type.

Comment on lines 13 to 16
bucket?: {
className: string;
icon: React.ComponentType<any>;
};
Copy link
Member

Choose a reason for hiding this comment

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

Can we have same definition of bucket as defined in other files. I think there should be centralised definition of Bucket for all the buckets variable passed in the components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but it showed errors when I used the same definition. That's why I made some props optional for buckets in some files and made some props compulsory for other files. Consequently, I created 2-3 different definitions for buckets for different files, but they are essentially the same; only the props are optional in some files.


return (
<LinkComponent href={item.comingSoon ? '' : item.href} key={index}>
<a
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
<a
<span

Instead of a tag, you can use span 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.

it is throwing an error beacause span doesnt have target attribute


useEffect(() => {
switch (active) {
case 'All':
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 create a enum for these strings.

@vishvamsinh28
Copy link
Contributor Author

@akshatnema updated the pr

* @description Select component for form dropdown.
* @param {string} [props.className=''] - Additional CSS classes for the select element.
* @param {(value: string) => void} [props.onChange=() => {}] - Function to handle onChange event.
* @param {Array<{ value: string, text: string }>} props.options - Array of options for the select dropdown.
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 {Array<{ value: string, text: string }>} props.options - Array of options for the select dropdown.
* @param {Option[]} props.options - Array of options for the select dropdown.

Comment on lines 39 to 41
* @param checks Array of filter criteria objects.
* @param data Array of data objects to filter.
* @param setFilters Function to update the filters.
Copy link
Member

Choose a reason for hiding this comment

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

Define type of these parameters in jsdocs as well. Applicable everywhere.

Comment on lines 40 to 55
const serializedBuckets: SerializedBuckets = buckets.reduce(
(acc, bucket) => {
acc[bucket.name || ''] = {
...bucket,
className: `${bucket.className || ''} ${bucket.borderClassName || ''}`
};

return acc;
},
{
welcome: {
icon: IconHome,
className: 'bg-gray-300 border-gray-300'
}
} as SerializedBuckets
);
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment here, explaining what we are doing here.

*/
if ((props.href && i18nPaths[language] && !i18nPaths[language].includes(href)) || href.includes('http', 0)) {
return (
<Link href={href} passHref legacyBehavior>
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 legacyBehavior everywhere?

@@ -31,6 +31,7 @@ export default function TextLink({ href, className = '', target = '_blank', chil
className={classNames}
id={id}
data-testid='TextLink-href'
legacyBehavior
Copy link
Member

Choose a reason for hiding this comment

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

Try to avoid legacyBehavior for Link tag.

@vishvamsinh28
Copy link
Contributor Author

@akshatnema updated the pr

akshatnema
akshatnema previously approved these changes Mar 28, 2024
@vishvamsinh28
Copy link
Contributor Author

@akshatnema package-lock updated

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 769d4ef into asyncapi:migrate-ts Mar 29, 2024
14 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.

4 participants