-
-
Notifications
You must be signed in to change notification settings - Fork 637
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
feat: migrate navigation #2765
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
components/ClickableLogo.tsx
Outdated
|
||
/** | ||
* @description Renders a logo that is clickable and redirects to a specified URL. | ||
* @param {ClickableLogoProps} props - The props for the component. |
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.
Define all params in jsdoc instead of the whole interface
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.
Do this for all components.
components/ClickableLogo.tsx
Outdated
<Link href={href}> | ||
<a className={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.
Add the legacyBehavior
prop to Link
component or merge the anchor to Link. https://nextjs.org/docs/messages/invalid-new-link-with-extra-anchor
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.
Instead of using legacyBehavior
, replace the a
with span
tag.
components/data/buckets.tsx
Outdated
link: string; | ||
className: string; | ||
borderClassName: string; | ||
Icon: React.ComponentType<any>; |
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.
Why this key is duplicated when there already exists an icon
key with the same value?
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.
Instead of using React.ComponentType<any>
, you can define a IconType in types
folder as React.ReactElement
.
types/form/Select.ts
Outdated
className?: string; | ||
onChange?: (selected: string) => void; | ||
options: Option[]; | ||
selected?: string; |
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.
selected?: string; | |
selected: string; |
@akshatnema updated the navigation pr |
onChange = () => {}, | ||
options = [], | ||
selected | ||
}: SelectProps) { |
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.
Why are we defining LanguageSelect
props with SelectProps
?
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.
because they both have same props
post: { | ||
slug: string; | ||
}; |
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.
Define post using Post type.
bucket?: { | ||
className: string; | ||
icon: React.ComponentType<any>; | ||
}; |
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 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.
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.
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.
components/navigation/MenuBlocks.tsx
Outdated
|
||
return ( | ||
<LinkComponent href={item.comingSoon ? '' : item.href} key={index}> | ||
<a |
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.
<a | |
<span |
Instead of a
tag, you can use span
tag.
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.
it is throwing an error beacause span doesnt have target attribute
|
||
useEffect(() => { | ||
switch (active) { | ||
case 'All': |
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 can create a enum for these strings.
@akshatnema updated the pr |
components/form/Select.tsx
Outdated
* @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. |
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.
* @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. |
components/helpers/applyFilter.ts
Outdated
* @param checks Array of filter criteria objects. | ||
* @param data Array of data objects to filter. | ||
* @param setFilters Function to update the filters. |
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.
Define type of these parameters in jsdocs as well. Applicable everywhere.
components/navigation/DocsNav.tsx
Outdated
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 | ||
); |
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.
Add a comment here, explaining what we are doing here.
components/link.tsx
Outdated
*/ | ||
if ((props.href && i18nPaths[language] && !i18nPaths[language].includes(href)) || href.includes('http', 0)) { | ||
return ( | ||
<Link href={href} passHref legacyBehavior> |
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.
Why are we adding legacyBehavior
everywhere?
components/typography/TextLink.tsx
Outdated
@@ -31,6 +31,7 @@ export default function TextLink({ href, className = '', target = '_blank', chil | |||
className={classNames} | |||
id={id} | |||
data-testid='TextLink-href' | |||
legacyBehavior |
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.
Try to avoid legacyBehavior
for Link tag.
@akshatnema updated the pr |
@akshatnema package-lock updated |
/rtm |
migrated navigation