fix: improve visibility of newsletter input fields #4837
Conversation
SyncWithRaj
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
📝 WalkthroughWalkthroughReworked the newsletter form container in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SyncWithRaj
changed the title
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4837--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
components/NewsletterSubscribe.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/NewsletterSubscribe.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Lint PR title
- GitHub Check: Notify-triagers
- GitHub Check: automerge-for-humans
- GitHub Check: remove-ready-label
- GitHub Check: Lighthouse CI
🔇 Additional comments (1)
components/NewsletterSubscribe.tsx (1)
141-141: Verify placeholder text contrast meets WCAG standards.The
placeholder-gray-500onbg-gray-100should provide sufficient contrast, but this should be verified to ensure compliance with WCAG AA standards (minimum 4.5:1 contrast ratio for normal text, though placeholders may fall under a different category).Test the newsletter section across different devices and verify placeholder text is easily readable by users with visual impairments, as specified in the PR objectives.
SyncWithRaj
changed the title
components/NewsletterSubscribe.tsx
Outdated
| <form | ||
| className=" | ||
| w-full flex flex-col gap-4 md:flex-row | ||
| [&>div]:!w-full md:[&>div]:!w-auto | ||
| [&_input]:!w-full | ||
| [&_input]:!border | ||
| [&_input]:!border-purple-600 | ||
| [&_input]:!bg-gray-100 | ||
| [&_input]:!placeholder-gray-500 | ||
| [&_input]:!pl-4 | ||
| [&_input]:!py-3 | ||
| [&_input]:rounded | ||
| " | ||
| onSubmit={handleSubmit} | ||
| > | ||
| <InputBox | ||
| inputType={InputTypes.TEXT} | ||
| inputName="name" | ||
| placeholder={ready ? t('nameInput') : 'Your name'} | ||
| inputValue={name} | ||
| setInput={setName} | ||
| /> | ||
|
|
||
| <InputBox | ||
| inputType={InputTypes.EMAIL} | ||
| inputName="email" | ||
| placeholder={ready ? t('emailInput') : 'Your email'} |
There was a problem hiding this comment.
@SyncWithRaj Revert the unnecessary changes in the package-lock file and fix the indentation. Also, please have a look at the CodeRabbit suggestion.
There was a problem hiding this comment.
Thanks for pointing this out.
The package-lock.json change was unintentional (npm install). I've reverted it to match upstream and updated the PR.
Please let me know if everything looks good now.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/NewsletterSubscribe.tsx (1)
136-151: Style conflicts with InputBox's built-in classes.The form's
[&_input]selectors conflict withInputBox's internal className (w-full,md:flex-1,mt-2,md:mt-0,rounded-md). This creates unpredictable styling behavior, particularly for responsive width (w-fullvsmd:flex-1) and spacing.Additionally, the form only applies
[&_input]:w-fullwithout responsive variants, which may prevent the intended desktop row layout where inputs should not be full-width.🔎 Recommended approach: modify InputBox to accept className
The cleanest solution is to modify
InputBoxto accept and merge aclassNameprop:In
components/InputBox.tsx:-export default function InputBox({ inputType, inputName, placeholder, inputValue, setInput }: InputBoxProps) { +export default function InputBox({ inputType, inputName, placeholder, inputValue, setInput, className = '' }: InputBoxProps) { return ( <input type={inputType} name={inputName} placeholder={placeholder} value={inputValue} onChange={(e) => setInput(e.target.value)} - className='form-input mt-2 block w-full rounded-md sm:text-sm sm:leading-5 md:mt-0 md:flex-1' + className={`form-input block rounded-md sm:text-sm sm:leading-5 ${className}`} required data-testid={`NewsletterSubscribe-${inputType}-input`} /> ); }Then in
NewsletterSubscribe.tsx:-<form - className=" - w-full flex flex-col gap-4 md:flex-row - [&_input]:w-full - [&_input]:border - [&_input]:border-primary-600 - [&_input]:bg-gray-100 - [&_input]:placeholder-gray-500 - [&_input]:pl-4 - [&_input]:py-3 - [&_input]:rounded-md - [&_input]:focus:outline-none - [&_input]:focus:ring-2 - [&_input]:focus:ring-primary-400 - - dark:[&_input]:bg-gray-300 - dark:[&_input]:border-primary-500 - dark:[&_input]:placeholder-black - " - onSubmit={handleSubmit} -> +<form className="w-full flex flex-col gap-4 md:flex-row" onSubmit={handleSubmit}> <InputBox inputType={InputTypes.TEXT} inputName="name" placeholder={ready ? t('nameInput') : 'Your name'} inputValue={name} setInput={setName} + className="w-full md:flex-1 border border-primary-600 bg-gray-100 placeholder-gray-500 pl-4 py-3 focus:outline-none focus:ring-2 focus:ring-primary-400 dark:bg-gray-300 dark:border-primary-500 dark:placeholder-black" /> <InputBox inputType={InputTypes.EMAIL} inputName="email" placeholder={ready ? t('emailInput') : 'Your email'} inputValue={email} setInput={setEmail} + className="w-full md:flex-1 border border-primary-600 bg-gray-100 placeholder-gray-500 pl-4 py-3 focus:outline-none focus:ring-2 focus:ring-primary-400 dark:bg-gray-300 dark:border-primary-500 dark:placeholder-black" />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/NewsletterSubscribe.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/NewsletterSubscribe.tsx
🧬 Code graph analysis (1)
components/NewsletterSubscribe.tsx (2)
components/InputBox.tsx (1)
InputBox(8-21)components/buttons/Button.tsx (1)
Button(53-106)
🔇 Additional comments (2)
components/NewsletterSubscribe.tsx (2)
144-146: Excellent focus state implementation.The focus styles (
focus:outline-none,focus:ring-2,focus:ring-primary-400) improve keyboard navigation and accessibility. Using the primary color for the focus ring maintains design system consistency.
133-176: Fix the responsive input width conflict in the form styling.The form applies
[&_input]:w-fullunconditionally, which prevents proper flex-row layout on desktop. Whenmd:flex-rowactivates at 768px+, the inputs remainw-fulland cannot fit alongside each other in a single row.Change the form className to use responsive input width:
Suggested fix
Replace
[&_input]:w-fullwithmd:[&_input]:w-auto md:[&_input]:flex-1so inputs:
- Remain full width on mobile (
w-full)- Share available space evenly on desktop (
flex-1)- Align properly with the button (
md:flex-1)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/NewsletterSubscribe.tsx (1)
148-150: Dark mode styles won't activate without applying thedarkclass.The
dark:[&_input]variants require a parent element with thedarkclass to take effect. The component'sdarkprop is used for text colors (lines 57-58) but is never applied as a class to enable these input styles.🔎 Proposed fix: conditionally apply dark class to form
- <form - className=" - w-full flex flex-col gap-4 md:flex-row + <form + className={` + ${dark ? 'dark' : ''} + w-full flex flex-col gap-4 md:flex-row [&_input]:w-full [&_input]:border [&_input]:border-primary-600 [&_input]:bg-gray-100 [&_input]:placeholder-gray-500 [&_input]:pl-4 [&_input]:py-3 [&_input]:rounded-md [&_input]:focus:outline-none [&_input]:focus:ring-2 [&_input]:focus:ring-primary-400 dark:[&_input]:bg-gray-300 dark:[&_input]:border-primary-500 dark:[&_input]:placeholder-black - " + `} onSubmit={handleSubmit} >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/NewsletterSubscribe.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/NewsletterSubscribe.tsx
🧬 Code graph analysis (1)
components/NewsletterSubscribe.tsx (2)
components/InputBox.tsx (1)
InputBox(8-21)components/buttons/Button.tsx (1)
Button(53-106)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (1)
components/NewsletterSubscribe.tsx (1)
133-153: Form structure and responsive layout improvements look good.The form styling addresses the PR objectives well:
- The
[&_input]selectors correctly target the<input>elements rendered byInputBox- Responsive layout (column on mobile, row on desktop with
md:flex-row) matches requirements- Border, background, padding, and placeholder contrast improvements enhance visibility
- Focus states (
focus:ring-2) improve accessibilityHowever, note that the dark mode styles (lines 148-150) won't activate without additional changes—see separate comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/NewsletterSubscribe.tsx (1)
173-178: CRITICAL: Removehref='undefined'to fix form submission.The Button component has
href='undefined'on line 177. This string literal is truthy, causing the Button to render as a<Link>instead of a<button>element, which breaks form submission because links don't trigger theonSubmithandler.This issue was previously flagged and marked as addressed, but still appears in the current code.
🔎 Proposed fix
<Button type={ButtonType.SUBMIT} text={ready ? t('subscribeBtn') : 'Subscribe'} className="mt-2 w-full md:mr-2 md:mt-0 md:flex-1" - href='undefined' />
🧹 Nitpick comments (1)
components/NewsletterSubscribe.tsx (1)
133-155: Consider extracting input styles to improve maintainability.The form's className string is extensive and could benefit from extraction. While the current implementation is functional and the
[&_input]descendant combinator correctly targets the InputBox elements, consider moving these styles to a separate constant or CSS module for better readability and maintenance.🔎 Example refactor using a constant
+const inputStyles = ` + [&_input]:w-full + [&_input]:border + [&_input]:border-primary-600 + [&_input]:bg-gray-100 + [&_input]:placeholder-gray-500 + [&_input]:pl-4 + [&_input]:py-3 + [&_input]:rounded-md + [&_input]:focus:outline-none + [&_input]:focus:ring-2 + [&_input]:focus:ring-primary-400 + dark:[&_input]:bg-gray-800 + dark:[&_input]:border-primary-500 + dark:[&_input]:placeholder-gray-400 +`; + <form className={` - w-full flex flex-col gap-4 md:flex-row - ${dark ? 'dark' : ''} - - [&_input]:w-full - [&_input]:border - [&_input]:border-primary-600 - [&_input]:bg-gray-100 - [&_input]:placeholder-gray-500 - [&_input]:pl-4 - [&_input]:py-3 - [&_input]:rounded-md - [&_input]:focus:outline-none - [&_input]:focus:ring-2 - [&_input]:focus:ring-primary-400 - - dark:[&_input]:bg-gray-800 - dark:[&_input]:border-primary-500 - dark:[&_input]:placeholder-gray-400 + w-full flex flex-col gap-4 md:flex-row + ${dark ? 'dark' : ''} + ${inputStyles} `} onSubmit={handleSubmit} >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/NewsletterSubscribe.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
components/NewsletterSubscribe.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/NewsletterSubscribe.tsx
🪛 GitHub Actions: PR testing - if Node project
components/NewsletterSubscribe.tsx
[error] 133-179: Prettier formatting errors detected in NewsletterSubscribe.tsx (prettier/prettier, jsx-quotes). Run 'prettier --write' to fix code style issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/buttons/Button.tsx (3)
71-71: Suggest refactoring to avoid non-null assertion.The condition at line 71 correctly filters out cases where
hrefis absent or falsy, ensuring line 101 only executes whenhrefis truthy. However, the non-null assertion (href!) is a code smell that bypasses TypeScript's safety checks.Consider refactoring to make the href presence explicit and avoid the assertion:
🔎 Proposed refactor
- if (!('href' in props) || !props.href) { + const { href } = props; + + if (!href) { return ( <button {...(props as React.ButtonHTMLAttributes<HTMLButtonElement>)} type={type} className={ buttonSize === ButtonSize.SMALL ? smallButtonClasses : classNames } data-testid="Button-main" > {icon && iconPosition === ButtonIconPosition.LEFT && ( <span className="mr-2 inline-block" data-testid="Button-icon-left"> {icon} </span> )} <span className="inline-block">{text}</span> {icon && iconPosition === ButtonIconPosition.RIGHT && ( <span className="ml-2 inline-block" data-testid="Button-icon-right"> {icon} </span> )} </button> ); } - const { href, ...linkProps } = - props as React.AnchorHTMLAttributes<HTMLAnchorElement>; + const { href: _href, ...linkProps } = + props as React.AnchorHTMLAttributes<HTMLAnchorElement>; return ( <Link - href={href!} + href={href} {...linkProps}Also applies to: 96-101
81-91: Consider extracting icon rendering logic to reduce duplication.The icon rendering logic is duplicated between the button and Link branches. While this works correctly, extracting it into a helper function or variable would improve maintainability.
🔎 Proposed refactor to reduce duplication
export default function Button({ text, type = ButtonType.BUTTON, target = '_self', icon, iconPosition = ButtonIconPosition.RIGHT, className = '', bgClassName = twMerge('bg-primary-500 hover:bg-primary-400'), textClassName = twMerge('text-white'), buttonSize, ...props }: IButtonProps): React.ReactElement { const smallButtonClasses = twMerge(`${bgClassName} ${textClassName} transition-all duration-500 ease-in-out rounded-md px-3 py-2 text-sm font-medium tracking-heading ${className || ''}`); const classNames = twMerge(`${bgClassName} ${textClassName} transition-all duration-500 ease-in-out rounded-md px-4 py-3 text-md font-semibold tracking-heading ${className || ''}`); + + const renderContent = () => ( + <> + {icon && iconPosition === ButtonIconPosition.LEFT && ( + <span className="mr-2 inline-block">{icon}</span> + )} + <span className="inline-block">{text}</span> + {icon && iconPosition === ButtonIconPosition.RIGHT && ( + <span className="ml-2 inline-block">{icon}</span> + )} + </> + ); if (!('href' in props) || !props.href) { return ( <button {...(props as React.ButtonHTMLAttributes<HTMLButtonElement>)} type={type} className={ buttonSize === ButtonSize.SMALL ? smallButtonClasses : classNames } data-testid="Button-main" > - {icon && iconPosition === ButtonIconPosition.LEFT && ( - <span className="mr-2 inline-block" data-testid="Button-icon-left"> - {icon} - </span> - )} - <span className="inline-block">{text}</span> - {icon && iconPosition === ButtonIconPosition.RIGHT && ( - <span className="ml-2 inline-block" data-testid="Button-icon-right"> - {icon} - </span> - )} + {renderContent()} </button> ); } const { href, ...linkProps } = props as React.AnchorHTMLAttributes<HTMLAnchorElement>; return ( <Link href={href!} {...linkProps} target={target} rel="noopener noreferrer" className={ buttonSize === ButtonSize.SMALL ? smallButtonClasses : classNames } data-testid="Button-link" > - {icon && iconPosition === ButtonIconPosition.LEFT && ( - <span className="mr-2 inline-block">{icon}</span> - )} - <span className="inline-block">{text}</span> - {icon && iconPosition === ButtonIconPosition.RIGHT && ( - <span className="ml-2 inline-block">{icon}</span> - )} + {renderContent()} </Link> ); }Also applies to: 110-116
41-44: Consider implementing a discriminated union type for better compile-time type safety.The current type union allows TypeScript to accept any combination of anchor and button HTML attributes regardless of the
hrefprop presence. While the runtime logic correctly branches based onhref(rendering<Link>or<button>accordingly), the type system doesn't enforce this constraint.Example of currently valid but potentially invalid code:
<Button href="/" type="submit" /> // Renders Link with type="submit" (button-only attribute)A discriminated union would restrict attributes based on
href:type IButtonProps = | { text: string; href: string; /* anchor attributes */ } | { text: string; href?: never; /* button attributes */ }All existing usages in the codebase follow correct patterns, but stricter typing would prevent future mistakes and improve IDE autocomplete.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/NewsletterSubscribe.tsxcomponents/buttons/Button.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/NewsletterSubscribe.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Learnt from: katara-Jayprakash
Repo: asyncapi/website PR: 4760
File: components/layout/GenericPostLayout.tsx:50-52
Timestamp: 2025-12-23T06:30:43.275Z
Learning: In the asyncapi/website repository, `GenericPostLayout` (components/layout/GenericPostLayout.tsx) is used for rendering about pages, not blog posts, even though it accepts a post typed as `IPosts['blog'][number]`. The type is reused for convenience. Blog posts use `BlogLayout.tsx` instead. When reviewing EditPageButton usage in GenericPostLayout, `contentType='about'` is the correct value.
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/buttons/Button.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/buttons/Button.tsx
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/buttons/Button.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/buttons/Button.tsx
🪛 GitHub Actions: PR testing - if Node project
components/buttons/Button.tsx
[error] 5-5: prettier/prettier: Replace multiline import formatting and remove trailing comma
[error] 8-8: prettier/prettier: Unexpected trailing comma in import list
[error] 12-12: prettier/prettier: Expected line break before closing bracket in type union
[error] 41-41: prettier/prettier: Delete trailing comma
[error] 64-64: prettier/prettier: Delete trailing comma
[error] 67-67: prettier/prettier: Remove extra newline in object/props
[error] 76-76: prettier/prettier: Use consistent quotation marks
[error] 79-79: prettier/prettier: Replace "Button-main" with 'Button-main'
[error] 82-82: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 104-104: prettier/prettier: Replace "noopener noreferrer" with 'noopener noreferrer'
[error] 105-105: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 108-108: prettier/prettier: Replace "Button-link" with 'Button-link'
[error] 110-110: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 110-110: prettier/prettier: Use single quotes for className values
[error] 111-111: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 113-113: prettier/prettier: Replace "inline-block" with 'inline-block'
[error] 113-113: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 114-114: prettier/prettier: Replace "ml-2 inline-block" with 'ml-2 inline-block'
[error] 114-114: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 124-124: prettier/prettier: Replace "Button-icon-left" with 'Button-icon-left' in data-testid
[error] 126-126: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 136-136: prettier/prettier: Replace "Button-main" with 'Button-main'
[error] 136-136: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 137-137: prettier/prettier: Use single quotes for className values
[error] 141-141: prettier/prettier: Replace "inline-block" with 'inline-block'
[error] 145-145: prettier/prettier: Replace "mb-8" with 'mb-8'
[error] 145-145: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 149-149: prettier/prettier: Replace multiline attributes with single-line format
[error] 180-180: prettier/prettier: Replace "name" with 'name'
[error] 180-180: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 188-188: prettier/prettier: Replace "email" with 'email'
[error] 188-188: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
[error] 197-197: prettier/prettier: Replace "mt-2 w-full md:mr-2 md:mt-0 md:flex-1" with 'mt-2 w-full md:mr-2 md:mt-0 md:flex-1'
[error] 197-197: prettier/prettier: Unexpected usage of doublequote. jsx-quotes
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (1)
components/buttons/Button.tsx (1)
5-9: Critical: Fix all prettier/prettier violations before merging.The pipeline reports 30+ prettier formatting errors throughout this file, including:
- Inconsistent quotation marks (double vs. single quotes)
- Trailing commas in imports and type definitions
- Line break and multiline formatting issues
- JSX attribute quote style violations
These violations will block the PR from merging.
🔧 Fix all formatting issues
Run prettier to auto-fix all formatting violations:
#!/bin/bash # Fix prettier violations in Button.tsx npx prettier --write components/buttons/Button.tsxAlso applies to: 38-44, 64-69, 71-118
⛔ Skipped due to learnings
Learnt from: akshatnema Repo: asyncapi/website PR: 3262 File: components/Avatar.tsx:35-44 Timestamp: 2024-10-11T10:46:58.882Z Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.Learnt from: akshatnema Repo: asyncapi/website PR: 3262 File: components/navigation/BlogPostItem.tsx:95-119 Timestamp: 2024-10-11T11:32:30.226Z Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.Learnt from: akshatnema Repo: asyncapi/website PR: 3262 File: components/newsroom/FeaturedBlogPost.tsx:90-92 Timestamp: 2024-10-11T07:38:35.745Z Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
3 participants
This PR improves the usability and responsiveness of the NewsletterSubscribe component.
Previously, the input fields lacked clear visual boundaries, had low placeholder contrast, and appeared misaligned or compressed on mobile devices. This update enhances clarity and consistency while preserving the existing design language.
What’s changed
Result
###How to test
Fixes #4836
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.