feat: alert component #3680
feat: alert component #3680
Conversation
🦋 Changeset detectedLatest commit: 0e6e8a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@abhinav700 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce a new Alert component to the project, incorporating dependencies from the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
wingkwong
changed the title
…per, adding isCloseable prop
By adding different variants for |
|
@abhinav700 you may try by resizing the browser to see and compare with input component. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
apps/docs/content/components/alert/is-closable.ts (2)
1-12: LGTM! Consider adding type annotations for better clarity.The
Appcomponent demonstrates the usage of theAlertcomponent with theisClosableprop effectively. The centering of the Alert using flex container is good for demonstration purposes.Consider adding type annotations to the
titleanddescriptionconstants for improved code clarity:- const title = "Email Sent!!"; - const description = "You will get a reply soon"; + const title: string = "Email Sent!!"; + const description: string = "You will get a reply soon";
14-20: LGTM! Consider adding type annotations for better type safety.The export structure is clean and allows for easy import of the example elsewhere.
To improve type safety, consider adding type annotations:
-const react = { +const react: Record<string, string> = { "/App.jsx": App, }; -export default { +export default { ...react, } as const;This change ensures that the
reactobject is correctly typed as a record of strings, and theas constassertion on the export prevents accidental mutations.packages/components/alert/__tests__/alert.test.tsx (3)
16-22: LGTM: Ref forwarding test is good, with room for improvement.The test correctly verifies that the ref is forwarded. To make it more robust, consider checking additional properties of the forwarded ref, such as its tagName or className.
Example enhancement:
expect(ref.current).not.toBeNull(); expect(ref.current?.tagName).toBe('DIV'); expect(ref.current).toHaveClass('nextui-alert');
74-84: LGTM: Alert closing behavior is well-tested, with room for improvement.This test case effectively verifies that the Alert component is removed from the DOM when the close button is clicked. To make it more robust, consider adding an assertion to check for the presence of the Alert before clicking the close button:
it("should close the alert when clicking on close button", () => { const { getByRole, queryByRole } = render(<Alert isClosable description={description} title={title} />); const alertBefore = queryByRole("alert"); expect(alertBefore).toBeInTheDocument(); const closeButton = getByRole("button"); act(() => { closeButton.click(); }); const alertAfter = queryByRole("alert"); expect(alertAfter).not.toBeInTheDocument(); });This enhancement ensures that the Alert is present before the close action and not present after, providing a more comprehensive test of the closing behavior.
1-85: Overall good test coverage with room for improvement.The test suite provides comprehensive coverage of the Alert component's core functionality, including rendering, ref forwarding, content display, and close behavior. The tests are well-structured and use appropriate testing practices.
To further enhance the test suite, consider adding tests for different variants of the Alert component (e.g., success, warning, error) to ensure proper styling and behavior for each variant.
Example test for variants:
it("should apply correct styles for different variants", () => { const variants = ["default", "success", "warning", "error"]; variants.forEach((variant) => { const { getByRole } = render( <Alert title={title} description={description} variant={variant} /> ); const alert = getByRole("alert"); expect(alert).toHaveClass(`alert-${variant}`); }); });This additional test would ensure that the Alert component correctly applies styles for different variants, improving the overall robustness of the test suite.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/docs/content/components/alert/index.ts (1 hunks)
- apps/docs/content/components/alert/is-closable.ts (1 hunks)
- packages/components/alert/tests/alert.test.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/content/components/alert/index.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/components/alert/__tests__/alert.test.tsx (3)
1-7: LGTM: Imports and constants are well-structured.The import statements and constant definitions are appropriate and follow good practices for test file organization.
34-58: LGTM: Comprehensive close button visibility tests.These test cases effectively cover different scenarios for close button visibility:
- When
isClosableis true- When
onCloseis provided- When neither
isClosableis true noronCloseis providedThe separate tests for each scenario provide clear and specific coverage. Good job on thorough testing of this feature.
60-72: LGTM: onClose functionality is well-tested.This test case effectively verifies that the
onClosefunction is called when the close button is clicked. The use ofjest.fn()to mock theonClosefunction andact()to wrap the button click event follows best practices for testing React components.
| it("should render correctly", () => { | ||
| const wrapper = render(<Alert description={description} title={title} />); | ||
|
|
||
| expect(() => wrapper.unmount()).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the render test.
While this test ensures the component doesn't throw errors during rendering and unmounting, it doesn't verify the presence of expected elements. Consider adding assertions to check if key elements are present in the rendered output.
Example improvement:
it("should render correctly", () => {
const { getByText } = render(<Alert description={description} title={title} />);
expect(getByText(title)).toBeInTheDocument();
expect(getByText(description)).toBeInTheDocument();
});| it("should display title and description when component is rendered", () => { | ||
| const wrapper = render(<Alert description={description} title={title} />); | ||
|
|
||
| const titleElement = wrapper.getByText(title); | ||
| const descriptionElement = wrapper.getByText(description); | ||
|
|
||
| expect(titleElement).toContainHTML(title); | ||
| expect(descriptionElement).toContainHTML(description); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using toHaveTextContent instead of toContainHTML.
The test correctly verifies the presence of title and description. However, toContainHTML might be overly specific and could break if the internal structure changes. Consider using toHaveTextContent for a more robust test:
expect(titleElement).toHaveTextContent(title);
expect(descriptionElement).toHaveTextContent(description);This change makes the test less dependent on the exact HTML structure while still verifying the correct content.
| export const AlertCloseIcon = (props: {color: string}) => { | ||
| switch (props.color) { | ||
| case "primary": | ||
| return <CloseIcon className="fill-current text-primary-200" height={20} width={20} />; | ||
|
|
||
| case "secondary": | ||
| return <CloseIcon className="fill-current text-secondary-200" height={20} width={20} />; | ||
|
|
||
| case "success": | ||
| return <CloseIcon className="fill-current text-success-200" height={20} width={20} />; | ||
|
|
||
| case "warning": | ||
| return <CloseIcon className="fill-current text-warning-200" height={20} width={20} />; | ||
|
|
||
| case "danger": | ||
| return <CloseIcon className="fill-current text-danger-200" height={20} width={20} />; | ||
|
|
||
| default: | ||
| return <CloseIcon className="fill-current text-default-400" height={20} width={20} />; | ||
| } |
There was a problem hiding this comment.
I believe the style for AlertIcon should also be defined in alert.ts.
There was a problem hiding this comment.
okay, I will look into that
There was a problem hiding this comment.
Just to be sure, You only mean the close icon right?
There was a problem hiding this comment.
I think the same approach should be applied to AlertIcon as well.
There was a problem hiding this comment.
I have implemented these changes.
There was a problem hiding this comment.
Sorry if I wasn't clear. I think className should not be defined here.
Instead, the intent is to call it like the following examples.
nextui/packages/components/avatar/src/avatar.tsx
Lines 45 to 47 in 765dac5
nextui/packages/components/modal/src/modal-content.tsx
Lines 57 to 59 in 765dac5
There was a problem hiding this comment.
Just to be sure,
Instead of defining the className seperately for closeIcon slot, I should define them in closeButton itself and avoid the use of getCloseIconProps, right?
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
packages/components/alert/src/alert-icons.tsx (1)
1-18: Overall assessment: Good foundation with room for improvementThe file introduces a new AlertIcon component with correct imports and a functional implementation. However, there are opportunities to enhance maintainability and consistency:
- The import statement is correct and uses shared packages as suggested.
- The AlertIcon component works as intended but could benefit from refactoring for improved maintainability.
- Consider implementing the suggested refactoring to use an object lookup instead of a switch statement.
- Move style definitions to
alert.tsfor better organization and consistency with other components.These improvements will make the code more maintainable and align it better with the project's coding standards.
packages/components/alert/src/alert.tsx (1)
37-59: Consider adding missing dependencies to useMemo.While the useMemo optimization is good, the dependency array for the baseWrapper useMemo hook is missing some dependencies that are used within the memoized function.
Consider updating the dependency array to include all used variables:
}, [ mainWrapper, isClosable, getCloseButtonProps, isVisible, domRef, getBaseProps, handleClose, color, onClose, + getCloseIconProps, ]);This ensures that the memoized value is correctly recalculated when any of its dependencies change.
packages/components/alert/src/use-alert.ts (1)
64-67: EnhancehandleClosefunction with optional chainingConsider using optional chaining when calling
onClosein thehandleClosefunction to ensure type safety:const handleClose = () => { setIsVisible(false); onClose?.(); };This change prevents potential runtime errors if
onCloseis undefined.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- apps/docs/content/components/alert/custom-impl.ts (1 hunks)
- apps/docs/content/docs/components/alert.mdx (1 hunks)
- packages/components/alert/src/alert-icons.tsx (1 hunks)
- packages/components/alert/src/alert.tsx (1 hunks)
- packages/components/alert/src/use-alert.ts (1 hunks)
- packages/core/theme/src/components/alert.ts (1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/alert.mdx
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... is true, a close button appears on the alert which closes it. It is set to false by ...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: You might be missing the article “the” here.
Context: ...e close button is visible regardless ofisClosableproperty. <CodeDemo title="...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~72-~72: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-right corner”?
Context: ...closeButton**: ThecloseButton, it is at the top-right corner of alert. - description: The descri...(ON_IN_THE_CORNER_2)
[uncategorized] ~72-~72: You might be missing the article “the” here.
Context: ...tton`, it is at the top-right corner of alert. - description: The description of ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (14)
packages/components/alert/src/alert-icons.tsx (1)
1-1: LGTM: Import statement is correctThe import statement correctly imports the necessary icons from the shared package, which aligns with the previous suggestions.
packages/components/alert/src/alert.tsx (4)
1-6: LGTM: Imports are appropriate and concise.The imports are well-organized and relevant to the component's functionality. Good job on keeping the imports clean and focused.
8-8: LGTM: Interface definition is clear and follows conventions.The
AlertPropsinterface correctly extendsUseAlertProps, promoting prop reuse and maintaining a clear structure. The use of PascalCase for the interface name adheres to TypeScript naming conventions.
10-62: LGTM: Well-structured and optimized Alert component.The Alert component is well-implemented using forwardRef and the custom useAlert hook. The use of useMemo for performance optimization is a good practice. The component handles conditional rendering and prop-based customization effectively.
64-66: LGTM: Proper display name and export.Setting the display name to "NextUI.Alert" is good for debugging. The default export is appropriate for the main component of the file.
packages/core/theme/src/components/alert.ts (1)
37-37:⚠️ Potential issueUse correct Tailwind class syntax for theme colors.
The class
text-[default-600]is using square brackets, which is typically used for arbitrary values. For theme colors, the brackets should be omitted.Apply this diff to fix the class name:
- description: ["text-[default-600]"], + description: ["text-default-600"],Likely invalid or redundant comment.
packages/components/alert/src/use-alert.ts (3)
96-100: Approval: 'closeButton' slot addedThe 'closeButton' slot has been successfully added to the
getCloseButtonPropsfunction, addressing a previous review comment. This addition enhances the customization options for the alert component.
4-4:⚠️ Potential issueUpdate import path for
AlertVariantPropsAs suggested in a previous review, consider updating the import of
AlertVariantPropsto use the package's main entry point instead of an internal path. This improves maintainability and reduces dependency on internal module structures.import type {AlertVariantProps} from "@nextui-org/theme";
24-24:⚠️ Potential issueMake
descriptionproperty requiredAs per a previous suggestion, the
descriptionproperty should be mandatory. Consider updating the property definition to reflect this:description: ReactNode;to
description: ReactNode; // RequiredThis change ensures that users of the component always provide a description for the alert.
apps/docs/content/docs/components/alert.mdx (5)
1-29: LGTM: Clear introduction and comprehensive installation instructions.The introduction succinctly defines alerts, and the installation section provides commands for various package managers, making it easy for users to integrate the component.
77-81: LGTM: Clear instructions for custom styling.The custom styles section effectively explains how to customize the Alert component using Tailwind CSS classes and provides a helpful code demo with highlighted lines.
83-92: LGTM: Comprehensive guidance for custom implementation.The custom implementation section effectively introduces the
useAlerthook for advanced customization. The inclusion of a CodeSandbox link is particularly helpful for users who want to experiment with their own implementations.
96-112: LGTM: Comprehensive API documentation.The API section provides well-structured and detailed tables for Alert Props and Alert Events, offering users a clear understanding of the component's capabilities and customization options.
1-112: Overall, excellent documentation with minor improvements suggested.The Alert component documentation is comprehensive, well-structured, and informative. It covers all necessary aspects, including installation, usage, customization, and API reference. The suggested improvements mainly focus on minor grammar issues and enhancing the structure in the usage section.
Key strengths:
- Clear introduction and installation instructions
- Comprehensive usage examples with code demos
- Detailed explanation of customization options
- Well-structured API reference
Suggested improvements:
- Address minor grammar issues in the usage and slots sections
- Enhance structure by using a Blockquote component in the usage section
Once these minor changes are implemented, the documentation will be even more user-friendly and professional.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... is true, a close button appears on the alert which closes it. It is set to false by ...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: You might be missing the article “the” here.
Context: ...e close button is visible regardless ofisClosableproperty. <CodeDemo title="...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~72-~72: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-right corner”?
Context: ...closeButton**: ThecloseButton, it is at the top-right corner of alert. - description: The descri...(ON_IN_THE_CORNER_2)
[uncategorized] ~72-~72: You might be missing the article “the” here.
Context: ...tton`, it is at the top-right corner of alert. - description: The description of ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
| * | ||
| * <div className={base()}> | ||
| * <AlertIcon/> | ||
| * <div className={mainWrapper()}> |
There was a problem hiding this comment.
Fix the typo in the JSDoc example.
There's a typo in the className prop in the example code. It should be className={mainWrapper()}.
Apply this diff to fix the typo:
- * <div className={mainWrapper())}>
+ * <div className={mainWrapper()}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * <div className={mainWrapper()}> | |
| * <div className={mainWrapper()}> |
| sm: { | ||
| base: "rounded-small", | ||
| }, | ||
| md: { | ||
| base: "rounded-medium", | ||
| }, | ||
| lg: { | ||
| base: "rounded-large", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Tailwind's default border-radius classes.
The current implementation uses custom classes for border-radius, which may not be defined in Tailwind's default configuration. To ensure consistency and prevent potential class mismatches, use Tailwind's built-in border-radius classes.
Apply this diff to adjust the border-radius classes:
sm: {
- base: "rounded-small",
+ base: "rounded-sm",
},
md: {
- base: "rounded-medium",
+ base: "rounded-md",
},
lg: {
- base: "rounded-large",
+ base: "rounded-lg",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sm: { | |
| base: "rounded-small", | |
| }, | |
| md: { | |
| base: "rounded-medium", | |
| }, | |
| lg: { | |
| base: "rounded-large", | |
| }, | |
| sm: { | |
| base: "rounded-sm", | |
| }, | |
| md: { | |
| base: "rounded-md", | |
| }, | |
| lg: { | |
| base: "rounded-lg", | |
| }, |
| base: ["flex flex-row w-[342px] h-17 p-3"], | ||
| title: ["text-medium font-normal block h-6"], | ||
| description: ["text-small font-normal h-5"], | ||
| mainWrapper: ["w-[268px] h-11 ms-5 flex flex-col box-border items-start"], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed dimensions with responsive alternatives.
Using fixed widths and heights can lead to responsiveness issues across different screen sizes. Consider using Tailwind's responsive utility classes or relative units instead.
Consider the following changes:
- Replace
w-[342px]with a responsive alternative likew-full max-w-smorw-11/12 max-w-sm. - Replace
h-17with a more flexible height likemin-h-[4.25rem]. - Replace
w-[268px]with a percentage-based width likew-4/5. - Replace
h-11withmin-h-[2.75rem].
Example:
- base: ["flex flex-row w-[342px] h-17 p-3"],
+ base: ["flex flex-row w-full max-w-sm min-h-[4.25rem] p-3"],
- mainWrapper: ["w-[268px] h-11 ms-5 flex flex-col box-border items-start"],
+ mainWrapper: ["w-4/5 min-h-[2.75rem] ms-5 flex flex-col box-border items-start"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| base: ["flex flex-row w-[342px] h-17 p-3"], | |
| title: ["text-medium font-normal block h-6"], | |
| description: ["text-small font-normal h-5"], | |
| mainWrapper: ["w-[268px] h-11 ms-5 flex flex-col box-border items-start"], | |
| base: ["flex flex-row w-full max-w-sm min-h-[4.25rem] p-3"], | |
| title: ["text-medium font-normal block h-6"], | |
| description: ["text-small font-normal h-5"], | |
| mainWrapper: ["w-4/5 min-h-[2.75rem] ms-5 flex flex-col box-border items-start"], |
| const InfoCircleIcon = `export const InfoCircleIcon = (props) => ( | ||
| <svg | ||
| fill="none" | ||
| height="24" | ||
| viewBox="0 0 24 24" | ||
| width="24" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| {...props} | ||
| > | ||
| <path | ||
| d="M12 22C17.51 22 22 17.51 22 12C22 6.49 17.51 2 12 2C6.49 2 2 6.49 2 12C2 17.51 6.49 22 12 22ZM12.75 16C12.75 16.41 12.41 16.75 12 16.75C11.59 16.75 11.25 16.41 11.25 16L11.25 11C11.25 10.59 11.59 10.25 12 10.25C12.41 10.25 12.75 10.59 12.75 11L12.75 16ZM11.08 7.62C11.13 7.49 11.2 7.39 11.29 7.29C11.39 7.2 11.5 7.13 11.62 7.08C11.74 7.03 11.87 7 12 7C12.13 7 12.26 7.03 12.38 7.08C12.5 7.13 12.61 7.2 12.71 7.29C12.8 7.39 12.87 7.49 12.92 7.62C12.97 7.74 13 7.87 13 8C13 8.13 12.97 8.26 12.92 8.38C12.87 8.5 12.8 8.61 12.71 8.71C12.61 8.8 12.5 8.87 12.38 8.92C12.14 9.02 11.86 9.02 11.62 8.92C11.5 8.87 11.39 8.8 11.29 8.71C11.2 8.61 11.13 8.5 11.08 8.38C11.03 8.26 11 8.13 11 8C11 7.87 11.03 7.74 11.08 7.62Z" | ||
| /> | ||
| </svg> | ||
| );`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor InfoCircleIcon to a proper React component file
The InfoCircleIcon is currently defined as a string literal containing JSX code. This approach may lead to issues with syntax highlighting, code analysis, and potential security risks if used improperly.
Consider refactoring the InfoCircleIcon to a proper React component in a separate file:
- Create a new file
InfoCircleIcon.jsxorInfoCircleIcon.tsx. - Define the component as a proper React functional component.
- Export the component as the default export.
Example:
import React from 'react';
const InfoCircleIcon = (props) => (
<svg
fill="none"
height="24"
viewBox="0 0 24 24"
width="24"
xmlns="http://www.w3.org/2000/svg"
{...props}
>
<path d="M12 22C17.51 22 22 17.51 22 12C22 6.49 17.51 2 12 2C6.49 2 2 6.49 2 12C2 17.51 6.49 22 12 22ZM12.75 16C12.75 16.41 12.41 16.75 12 16.75C11.59 16.75 11.25 16.41 11.25 16L11.25 11C11.25 10.59 11.59 10.25 12 10.25C12.41 10.25 12.75 10.59 12.75 11L12.75 16ZM11.08 7.62C11.13 7.49 11.2 7.39 11.29 7.29C11.39 7.2 11.5 7.13 11.62 7.08C11.74 7.03 11.87 7 12 7C12.13 7 12.26 7.03 12.38 7.08C12.5 7.13 12.61 7.2 12.71 7.29C12.8 7.39 12.87 7.49 12.92 7.62C12.97 7.74 13 7.87 13 8C13 8.13 12.97 8.26 12.92 8.38C12.87 8.5 12.8 8.61 12.71 8.71C12.61 8.8 12.5 8.87 12.38 8.92C12.14 9.02 11.86 9.02 11.62 8.92C11.5 8.87 11.39 8.8 11.29 8.71C11.2 8.61 11.13 8.5 11.08 8.38C11.03 8.26 11 8.13 11 8C11 7.87 11.03 7.74 11.08 7.62Z" />
</svg>
);
export default InfoCircleIcon;This change will improve code maintainability and reduce potential issues.
There was a problem hiding this comment.
@abhinav700 Understood, I'll disregard this suggestion.
(_/)
( ;_;)
/ >
| const App = `import React, {forwardRef, useMemo} from "react"; | ||
| import {useAlert} from "@nextui-org/react"; | ||
| import {InfoCircleIcon} from "./InfoCircleIcon"; | ||
| import {CloseIcon} from "./CloseIcon" | ||
| const styles = { | ||
| base: [ | ||
| "bg-slate-100", | ||
| "border", | ||
| "shadow", | ||
| "hover:bg-slate-200", | ||
| "focus-within:!bg-slate-100", | ||
| "dark:bg-slate-900", | ||
| "dark:hover:bg-slate-800", | ||
| "dark:border-slate-800", | ||
| "dark:focus-within:!bg-slate-900", | ||
| "cursor-pointer" | ||
| ], | ||
| title: [ | ||
| "text-base", | ||
| "text-slate-500", | ||
| "font-bold" | ||
| ], | ||
| description: [ | ||
| "text-base", | ||
| "text-slate-500", | ||
| ], | ||
| } | ||
| const MyAlert = forwardRef((props, ref) => { | ||
| const { | ||
| title, | ||
| description, | ||
| isClosable, | ||
| domRef, | ||
| handleClose, | ||
| getBaseProps, | ||
| getMainWrapperProps, | ||
| getDescriptionProps, | ||
| getTitleProps, | ||
| getCloseButtonProps, | ||
| color, | ||
| isVisible, | ||
| onClose, | ||
| getCloseIconProps, | ||
| } = useAlert({ | ||
| ...props, | ||
| ref, | ||
| // this is just for the example, the props bellow should be passed by the parent component | ||
| title: "Email Sent!!", | ||
| description: "You will get a reply soon", | ||
| // custom styles | ||
| classNames: { | ||
| ...styles, | ||
| }, | ||
| }); | ||
| const mainWrapper = useMemo(() => { | ||
| return ( | ||
| <div {...getMainWrapperProps()}> | ||
| {title && <div {...getTitleProps()}>{title}</div>} | ||
| <div {...getDescriptionProps()}>{description}</div> | ||
| </div> | ||
| ); | ||
| }, [title, description, getMainWrapperProps, getTitleProps, getDescriptionProps]); | ||
| const baseWrapper = useMemo(() => { | ||
| return isVisible ? ( | ||
| <div ref={domRef} {...getBaseProps()}> | ||
| <InfoCircleIcon /> | ||
| {mainWrapper} | ||
| {(isClosable || onClose) && ( | ||
| <button onClick={handleClose} {...getCloseButtonProps()}> | ||
| <CloseIcon {...getCloseIconProps()}/> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ) : null; | ||
| }, [ | ||
| mainWrapper, | ||
| isClosable, | ||
| getCloseButtonProps, | ||
| isVisible, | ||
| domRef, | ||
| getBaseProps, | ||
| handleClose, | ||
| color, | ||
| onClose | ||
| ]); | ||
| return <>{baseWrapper}</>; | ||
| }); | ||
| MyAlert.displayName = "MyAlert"; | ||
| export default MyAlert;`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor MyAlert to a proper React component file and remove hardcoded values
The MyAlert component is currently defined as a string literal, which can lead to various issues including difficulty in maintenance, lack of proper syntax highlighting, and potential security risks. Additionally, the component contains hardcoded values for title and description, limiting its reusability.
Consider the following improvements:
-
Refactor MyAlert into a separate React component file:
- Create a new file
MyAlert.jsxorMyAlert.tsx. - Move the component code into this file, removing the string literal wrapper.
- Properly import the required dependencies at the top of the file.
- Create a new file
-
Remove hardcoded values for title and description:
- Remove the following lines:
// this is just for the example, the props bellow should be passed by the parent component title: "Email Sent!!", description: "You will get a reply soon",
- Allow these props to be passed from the parent component instead.
- Remove the following lines:
-
Update import statements:
import React, { forwardRef, useMemo } from "react"; import { useAlert } from "@nextui-org/react"; import InfoCircleIcon from "./InfoCircleIcon"; import CloseIcon from "./CloseIcon";
Example of refactored MyAlert component:
import React, { forwardRef, useMemo } from "react";
import { useAlert } from "@nextui-org/react";
import InfoCircleIcon from "./InfoCircleIcon";
import CloseIcon from "./CloseIcon";
const styles = {
// ... (keep the styles object as is)
};
const MyAlert = forwardRef((props, ref) => {
const {
title,
description,
isClosable,
domRef,
handleClose,
getBaseProps,
getMainWrapperProps,
getDescriptionProps,
getTitleProps,
getCloseButtonProps,
color,
isVisible,
onClose,
getCloseIconProps,
} = useAlert({
...props,
ref,
classNames: {
...styles,
},
});
// ... (keep the rest of the component logic as is)
});
MyAlert.displayName = "MyAlert";
export default MyAlert;These changes will significantly improve code maintainability, reusability, and reduce potential issues.
| ## Slots | ||
|
|
||
| - **base**: Alert wrapper, it handles alignment, placement, and general appearance. | ||
| - **mainWrapper**: Wraps the `title` and `description` of the alert. | ||
| - **closeButton**: The `closeButton`, it is at the top-right corner of alert. | ||
| - **description**: The description of the alert. | ||
| - **title**: The title of the alert. | ||
| - **closeIcon**: close icon that is wrapped inside the `closeButton`. |
There was a problem hiding this comment.
Improve grammar in the slots section.
Fix the following minor grammar issues:
- Line 72: Change "it is at the top-right corner of alert" to "it is in the top-right corner of the alert".
- Line 75: Change "close icon that is wrapped" to "the close icon that is wrapped".
Apply these changes:
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 LanguageTool
[grammar] ~72-~72: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-right corner”?
Context: ...closeButton**: ThecloseButton, it is at the top-right corner of alert. - description: The descri...(ON_IN_THE_CORNER_2)
[uncategorized] ~72-~72: You might be missing the article “the” here.
Context: ...tton`, it is at the top-right corner of alert. - description: The description of ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
| ## Import | ||
|
|
||
|
|
||
| <ImportTabs | ||
| commands={{ | ||
| main: 'import {Alert} from "@nextui-org/react";', | ||
| individual: 'import {Alert} from "@nextui-org/alert";', | ||
| }} | ||
| /> | ||
|
|
||
| ## Usage | ||
| - It accepts `title` and `description` as props for the alert message. | ||
| - `title` is an optional property. | ||
|
|
||
| <CodeDemo title="Usage" files={alertContent.usage} /> | ||
|
|
||
| ### Radius | ||
|
|
||
| <CodeDemo title="Radius" files={alertContent.radius} /> | ||
|
|
||
| ### Colors | ||
|
|
||
| <CodeDemo title="Colors" files={alertContent.colors} /> | ||
|
|
||
|
|
||
| ### isClosable | ||
|
|
||
| If `isClosable` is true, a close button appears on the alert which closes it. | ||
| It is set to false by default. | ||
|
|
||
| > **Note**: If the `onClose` is passed, the close button is visible regardless of `isClosable` property. | ||
| <CodeDemo title="isClosable" files={alertContent.isClosable} /> |
There was a problem hiding this comment.
Improve clarity and structure in the usage section.
-
Fix grammar issues:
- Line 58: Change "a close button appears on the alert which closes it" to "a close button appears on the alert, which can be used to close it".
- Line 61: Add "the" before "isClosable property".
-
Enhance structure:
- Use a
<Blockquote>component for the information on lines 42-43 to improve visual hierarchy.
- Use a
Apply these changes:
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... is true, a close button appears on the alert which closes it. It is set to false by ...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: You might be missing the article “the” here.
Context: ...e close button is visible regardless ofisClosableproperty. <CodeDemo title="...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
@wingkwong I have added breakpoints to change the width and make it more responsive. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/docs/content/docs/components/alert.mdx (1)
95-111: Enhance API table readability.The API section provides comprehensive information about the Alert component's props and events. To improve readability, consider the following suggestion:
- Use a more compact format for the
colorprop options. For example:| Attribute | Type | Description | Default | | --------- | ---- | ----------- | ------- | | color | `"default"` \| `"primary"` \| `"secondary"` \| `"success"` \| `"warning"` \| `"danger"` | The alert color theme. | `"default"` |This format is more concise and easier to read, especially for props with multiple options.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/docs/content/docs/components/alert.mdx (1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/alert.mdx
[grammar] ~71-~71: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-right corner”?
Context: ...closeButton**: ThecloseButton, it is at the top-right corner of alert. - description: The descri...(ON_IN_THE_CORNER_2)
🔇 Additional comments (6)
apps/docs/content/docs/components/alert.mdx (6)
1-13: LGTM: Header section is well-structured and informative.The header section provides clear metadata and a concise description of the Alert component. The inclusion of the ComponentLinks is a good practice for navigation.
18-29: LGTM: Comprehensive installation instructions provided.The installation section covers multiple package managers, making it easy for users with different setups to install the component. The inclusion of a global install warning is a good practice for preventing potential issues.
31-39: LGTM: Clear import instructions provided.The import section effectively uses the ImportTabs component to demonstrate both main package and individual component import options, catering to different user preferences and project setups.
41-62: Improve clarity and fix minor grammar issues in the usage section.The usage section is informative, but there are a few areas that could be improved:
Lines 42-43: Consider using a
<Blockquote>component for better visual hierarchy:<Blockquote> It accepts `title` and `description` as props for the alert message. `title` is an optional property. </Blockquote>Line 58: Improve clarity by changing to:
"IfisClosableis true, a close button appears on the alert, which can be used to close it."Line 60: Add "the" before "isClosable property":
"If theonCloseis passed, the close button is visible regardless of theisClosableproperty."Additionally, consider adding brief explanations or examples for the "Radius" and "Colors" subsections to enhance user understanding of these customization options.
67-74: Fix minor grammar issue in the slots section.The slots section provides valuable information for customizing the Alert component. However, there's a minor grammar issue that should be addressed:
Line 71: Change "it is at the top-right corner of alert" to "it is in the top-right corner of the alert".
Line 74: For consistency, consider changing "close icon that is wrapped" to "The close icon that is wrapped".
These small changes will improve the overall clarity and consistency of the documentation.
🧰 Tools
🪛 LanguageTool
[grammar] ~71-~71: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-right corner”?
Context: ...closeButton**: ThecloseButton, it is at the top-right corner of alert. - description: The descri...(ON_IN_THE_CORNER_2)
76-91: LGTM: Comprehensive customization options provided.The custom styles and implementation section offers valuable information for advanced users:
- Clear instructions on using Tailwind CSS classes for customization.
- Introduction of the
useAlerthook for more complex implementations.- Appropriate use of CodeDemo components to illustrate the concepts.
This section effectively caters to users who need more control over the Alert component's appearance and behavior.
|
@abhinav700 i think the reason is you have the fixed width so it won't shrink when the window is resizing. You may try if you put two alert on the same row and resize the container. Adding breakpoint doesn't solve the full problem since it may not reach the breakpoint. |
I think my original code would work if I replace responsive.mp4 |
|
Let's discuss in discord. |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (7)
apps/docs/content/components/alert/colors.ts (1)
1-27: Overall assessment: Improvements needed to fully meet PR objectivesThe implementation successfully introduces the Alert component demo with various color options. However, there are several areas for improvement:
- Component structure: Consider refactoring to a separate
.tsxfile for better maintainability and type safety.- Dismissibility: Implement the ability for users to dismiss alerts, as mentioned in the PR objectives.
- Content management: Use constants or a configuration object for alert content to facilitate updates and potential internationalization.
- File extension consistency: Ensure the file extension in the
reactobject matches the actual file type.To fully meet the PR objectives and improve the overall quality of the implementation, please address the suggestions provided in the previous comments. This will enhance the component's functionality, maintainability, and alignment with the project's goals.
If you need any assistance in implementing these changes or would like to discuss alternative approaches, please don't hesitate to ask. We can also create GitHub issues to track these improvements if needed.
packages/components/alert/stories/alert.stories.tsx (2)
1-36: LGTM! Consider enhancing the decorator for better responsiveness.The imports and Storybook configuration look good. The decorator effectively centers the stories, which is great for presentation.
Consider updating the decorator to be more responsive:
decorators: [ (Story) => ( - <div className="flex items-center justify-center w-screen h-screen"> + <div className="flex items-center justify-center min-h-screen p-4"> <Story /> </div> ), ],This change ensures proper padding on smaller screens and uses
min-heightinstead of a fixed height for better responsiveness.
101-122: LGTM! Consider adding a comment about potential style conflicts.The CustomWithClassNames story effectively demonstrates how to apply custom styles to the Alert component using Tailwind CSS classes.
Consider adding a comment to warn about potential style conflicts:
export const CustomWithClassNames = { render: Template, args: { ...defaultProps, classNames: { // Note: These custom styles may override or conflict with the component's built-in styles. // Ensure to test thoroughly when applying custom styles. base: [ // ... (existing classes) ], // ... (other custom classes) }, }, };This comment will remind developers to be cautious when applying custom styles and to test thoroughly for any unintended side effects.
apps/docs/content/docs/components/alert.mdx (2)
41-62: Improve grammar and structure in the usage section.While the content is informative, there are a few areas for improvement:
- Line 43: Change "a optional" to "an optional" to fix the grammar.
- Line 58: Consider rephrasing to "If
isClosableis true, a close button appears on the alert, which can be used to dismiss it."- Lines 60-61: Use a
<Blockquote>component for the note about theonCloseprop to improve visual hierarchy.Consider applying these changes:
-- `title` is a optional property. -- `title` is an optional property. -If `isClosable` is true, a close button appears on the alert, which can be used to close it. -If `isClosable` is true, a close button appears on the alert, which can be used to dismiss it. -> **Note**: If the `onClose` is passed, the close button is visible regardless of the `isClosable` property. +<Blockquote> + If the `onClose` prop is passed, the close button is visible regardless of the `isClosable` property. +</Blockquote>
67-92: Improve grammar in the slots section and LGTM for content.The slots section provides valuable information about customizing the Alert component. However, there are a few grammar issues to address:
- Line 71: Change "it is at the top-right corner of alert" to "it is in the top-right corner of the alert".
- Line 74: Change "close icon that is wrapped" to "the close icon that is wrapped".
- Line 75: Change "icon that appears at the top-left corner" to "icon that appears in the top-left corner".
Consider applying these changes:
-- **closeButton**: The `closeButton`, it is at the top-right corner of alert. -- **closeButton**: The `closeButton`, it is in the top-right corner of the alert. -- **closeIcon**: close icon that is wrapped inside the `closeButton`. -- **closeIcon**: The close icon that is wrapped inside the `closeButton`. -- **alertIcon**: icon that appears at the top-left corner. -- **alertIcon**: Icon that appears in the top-left corner.The examples for custom styles and custom implementation are well-presented and provide good guidance for developers looking to customize the Alert component.
🧰 Tools
🪛 LanguageTool
[grammar] ~71-~71: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-right corner”?
Context: ...closeButton**: ThecloseButton, it is at the top-right corner of alert. - description: The descri...(ON_IN_THE_CORNER_2)
[grammar] ~75-~75: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-left corner”?
Context: ...on`. - alertIcon: icon that appears at the top-left corner. ### Custom Styles You can customize ...(ON_IN_THE_CORNER_2)
packages/components/alert/src/alert.tsx (1)
84-84: Simplify the return statement by removing the unnecessary fragmentSince
baseWrapperis a single element, you can return it directly without wrapping it in a fragment.Apply this diff:
- return <>{baseWrapper}</>; + return baseWrapper;packages/components/alert/src/use-alert.ts (1)
17-17: Ensure consistent capitalization in commentsSome comments begin with lowercase letters (e.g., "title of the alert message", "whether the alert can be closed by user", "function which is called when close button is clicked"). For consistency and readability, consider starting all comments with a capital letter.
Also applies to: 27-27, 32-32
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- apps/docs/content/components/alert/colors.ts (1 hunks)
- apps/docs/content/components/alert/custom-impl.ts (1 hunks)
- apps/docs/content/components/alert/radius.ts (1 hunks)
- apps/docs/content/docs/components/alert.mdx (1 hunks)
- packages/components/alert/src/alert.tsx (1 hunks)
- packages/components/alert/src/use-alert.ts (1 hunks)
- packages/components/alert/stories/alert.stories.tsx (1 hunks)
- packages/core/theme/src/components/alert.ts (1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/alert.mdx
[grammar] ~71-~71: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-right corner”?
Context: ...closeButton**: ThecloseButton, it is at the top-right corner of alert. - description: The descri...(ON_IN_THE_CORNER_2)
[grammar] ~75-~75: The usual preposition to use with “corner” is “in”, not “on”. Did you mean “in the top-left corner”?
Context: ...on`. - alertIcon: icon that appears at the top-left corner. ### Custom Styles You can customize ...(ON_IN_THE_CORNER_2)
🔇 Additional comments (9)
apps/docs/content/components/alert/colors.ts (1)
25-27: LGTM: Export structure is appropriateThe export structure using object spread is correct and allows for easy expansion if more components need to be added in the future. This approach is flexible and maintainable.
apps/docs/content/docs/components/alert.mdx (5)
1-13: LGTM: Title and description are clear and concise.The title, description, and component links are well-structured and provide a good introduction to the Alert component.
18-29: LGTM: Comprehensive installation instructions provided.The installation section covers multiple package managers and includes a global install warning, which is appropriate for a component library.
31-39: LGTM: Clear import instructions provided.The import section clearly shows how to import the Alert component from both the main package and the individual component package.
96-107: LGTM: Comprehensive API documentation.The API section provides a clear and well-structured table of props for the Alert component, including descriptions, types, and default values. This information is valuable for developers implementing the component.
108-112: LGTM: Clear events documentation.The Events section succinctly describes the
onCloseevent for the Alert component, providing developers with the necessary information for handling alert dismissals.packages/components/alert/src/alert.tsx (1)
17-32: Verify all necessary props are destructured fromuseAlertEnsure that all required properties returned by
useAlertare destructured. Missing props could lead to unexpected behavior.Run the following script to list all exported properties from
useAlert:Compare the list of properties with the destructured variables to ensure none are missing.
✅ Verification successful
All necessary props are correctly destructured from
useAlert.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List all exports from 'useAlert' to ensure all necessary props are destructured. # Test: List all returned properties from 'useAlert'. Expect: No missing properties in destructuring. rg 'return {(.*)}' packages/components/alert/src/use-alert.ts | sed -E 's/return {(.*)}/\1/' | tr ',' '\n' | sortLength of output: 273
Script:
#!/bin/bash # Description: Extract all properties returned by 'useAlert' using ast-grep. ast-grep --lang typescript --pattern 'return { $$$ }' packages/components/alert/src/use-alert.tsLength of output: 2774
packages/core/theme/src/components/alert.ts (2)
36-78:⚠️ Potential issueVerify the custom color utilities used in color variants.
The color variants utilize classes like
bg-default-100,text-default-600,text-primary,text-primary-200, etc. These are not part of Tailwind CSS's default color palette.Please ensure that these color utilities are defined in your Tailwind CSS configuration under the
theme.colorssection. If they are not defined, the styles may not render as expected. Alternatively, consider using Tailwind's default colors likebg-blue-100,text-gray-600, etc.Run the following script to check for custom color definitions:
#!/bin/bash # Description: Verify custom colors in Tailwind CSS configuration. # Test: Search for custom colors such as 'default', 'primary', 'secondary', 'success', 'warning', 'danger'. # Expected: Definitions of the custom colors in the Tailwind config. # Find Tailwind config files and search for the custom color definitions. fd --type file --regex 'tailwind\.config\.(js|ts)' --exec cat {} \; | \ grep -E 'colors.*(default|primary|secondary|success|warning|danger)'
25-34:⚠️ Potential issueVerify the usage of potential non-standard Tailwind CSS classes in slots.
The slots are using classes like
text-medium,text-small,min-h-17, andms-5. Some of these may not exist in Tailwind CSS's default configuration:
text-mediumandtext-smallare not standard font size utilities.min-h-17may not be a default spacing utility.ms-5is the margin-start utility for RTL support; ensure it's configured correctly.Please verify that these classes are defined in your Tailwind CSS configuration or consider using standard classes like
text-base,text-sm,min-h-16, etc., for better compatibility and maintainability.Run the following script to check if these custom classes are defined:
| const App = `import {Alert} from "@nextui-org/react"; | ||
| export default function App() { | ||
| const title = "Email Sent!!"; | ||
| const description = "You will get a reply soon"; | ||
| return ( | ||
| <div className="flex items-center justify-center w-screen"> | ||
| <div className="flex flex-col w-full"> | ||
| {["none", "sm", "md", "lg", "full"].map((radius) => ( | ||
| <div key={radius} className="w-full flex items-center my-3"> | ||
| <span className="mx-4 text-md">{radius}</span> | ||
| <Alert title={title} description={description} radius={radius} /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor component structure and improve naming
The current implementation using a template literal string for the entire component can lead to maintainability issues. Consider refactoring to a standard TypeScript/React component structure. Additionally, the component name "App" is too generic. A more descriptive name like "AlertRadiusExample" would better convey its purpose.
Refactor the code to:
import {Alert} from "@nextui-org/react";
export default function AlertRadiusExample() {
// ... component code
}This change will improve type checking, IDE support, and overall code quality.
| const title = "Email Sent!!"; | ||
| const description = "You will get a reply soon"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance flexibility with props for alert content
The current implementation uses hardcoded values for the alert title and description. To improve reusability and flexibility, consider accepting these as props.
Refactor the component to accept props:
interface AlertRadiusExampleProps {
title?: string;
description?: string;
}
export default function AlertRadiusExample({
title = "Email Sent!!",
description = "You will get a reply soon"
}: AlertRadiusExampleProps) {
// ... rest of the component
}This change allows for easier customization when using the component in different contexts.
| {["none", "sm", "md", "lg", "full"].map((radius) => ( | ||
| <div key={radius} className="w-full flex items-center my-3"> | ||
| <span className="mx-4 text-md">{radius}</span> | ||
| <Alert title={title} description={description} radius={radius} /> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Showcase more Alert component features
While the current implementation effectively demonstrates different radius options, it doesn't fully showcase the Alert component's capabilities. Consider expanding the example to include more features and variants.
Enhance the example by including:
- Different alert types (success, warning, error)
- Variants (if supported, e.g., filled, outlined)
- Customizable colors
- Dismissible alerts with close buttons
- Alerts with icons
Example implementation:
const alertTypes = [
{ type: "success", title: "Success", description: "Operation completed successfully" },
{ type: "warning", title: "Warning", description: "Please review your input" },
{ type: "error", title: "Error", description: "An error occurred" },
];
// In the render method
{alertTypes.map((alert) => (
<Alert
key={alert.type}
type={alert.type}
title={alert.title}
description={alert.description}
radius={radius}
variant="filled"
isClosable
icon={getIconForType(alert.type)}
/>
))}This enhancement will provide a more comprehensive demonstration of the Alert component's features.
| const react = { | ||
| "/App.jsx": App, | ||
| }; | ||
|
|
||
| export default { | ||
| ...react, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the export structure
The current export structure seems overly complex for a single component example. Consider simplifying it to directly export the component.
Replace the current export structure with a simple default export:
export default AlertRadiusExample;This change will make the file more straightforward and easier to import and use in other parts of the documentation or examples.
| const App = `import {Alert} from "@nextui-org/react"; | ||
| export default function App() { | ||
| const title = "Email Sent!!"; | ||
| const description = "You will get a reply soon"; | ||
| return ( | ||
| <div className="flex items-center justify-center w-screen"> | ||
| <div className="flex flex-col w-full"> | ||
| {["default", "primary", "secondary", "success", "warning", "danger"].map((color) => ( | ||
| <div key={color} className="w-full flex items-center my-3"> | ||
| <span className="mx-4 text-md">{color}</span> | ||
| <Alert title={title} description={description} color={color} /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the component structure and implement dismissibility
The current implementation has several areas for improvement:
- The component is defined as a string template literal, which can lead to issues with syntax highlighting, type checking, and maintainability.
- The title and description are hardcoded, making it difficult to update or internationalize the content.
- The Alert component doesn't implement dismissibility, which was mentioned in the PR objectives.
Consider the following improvements:
- Create a separate
.tsxfile for theAppcomponent:
import React from 'react';
import {Alert} from "@nextui-org/react";
const ALERT_CONTENT = {
title: "Email Sent!!",
description: "You will get a reply soon"
};
export default function App() {
const { title, description } = ALERT_CONTENT;
return (
<div className="flex items-center justify-center w-screen">
<div className="flex flex-col w-full">
{["default", "primary", "secondary", "success", "warning", "danger"].map((color) => (
<div key={color} className="w-full flex items-center my-3">
<span className="mx-4 text-md">{color}</span>
<Alert
title={title}
description={description}
color={color}
onClose={() => console.log(`${color} alert closed`)}
/>
</div>
))}
</div>
</div>
);
}- In this file, import and use the
Appcomponent:
import App from './App';
export default {
"/App.tsx": App,
};These changes will improve type safety, syntax highlighting, and maintainability while also addressing the PR objectives.
Would you like assistance in implementing these changes or creating a new GitHub issue to track this refactoring task?
| export const Default = { | ||
| render: Template, | ||
| args: { | ||
| ...defaultProps, | ||
| }, | ||
| }; | ||
| export const Color = { | ||
| render: ColorTemplate, | ||
| args: { | ||
| ...defaultProps, | ||
| }, | ||
| }; | ||
| export const Radius = { | ||
| render: RadiusTemplate, | ||
| args: { | ||
| ...defaultProps, | ||
| }, | ||
| }; | ||
| export const isClosable = { | ||
| render: Template, | ||
| args: { | ||
| ...defaultProps, | ||
| isClosable: true, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the isClosable story to demonstrate both variants.
The story exports effectively cover different aspects of the Alert component. However, the isClosable story could be more informative.
Consider updating the isClosable story to show both closable and non-closable variants:
export const IsClosable = {
render: (args) => (
<div className="flex flex-col gap-4">
<div>
<span className="block mb-2 font-semibold">isClosable: true</span>
<Alert {...args} isClosable={true} />
</div>
<div>
<span className="block mb-2 font-semibold">isClosable: false</span>
<Alert {...args} isClosable={false} />
</div>
</div>
),
args: {
...defaultProps,
},
};This change will provide a clearer comparison between closable and non-closable Alert components.
| const baseWrapper = useMemo(() => { | ||
| return isVisible ? ( | ||
| <div ref={domRef} {...getBaseProps()}> | ||
| {alertIcon} | ||
| {mainWrapper} | ||
| {(isClosable || onClose) && ( | ||
| <button onClick={handleClose} {...getCloseButtonProps()}> | ||
| <CloseIcon {...getCloseIconProps()} height={20} width={20} /> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ) : null; | ||
| }, [ | ||
| mainWrapper, | ||
| isClosable, | ||
| getCloseButtonProps, | ||
| isVisible, | ||
| domRef, | ||
| getBaseProps, | ||
| handleClose, | ||
| color, | ||
| onClose, | ||
| ]); |
There was a problem hiding this comment.
Include missing dependencies in baseWrapper's useMemo
The baseWrapper memoization depends on alertIcon and getCloseIconProps, but they are not included in the dependency array. This can lead to inconsistencies if these values change.
Apply this diff to include the missing dependencies:
}, [
+ alertIcon,
+ getCloseIconProps,
mainWrapper,
isClosable,
getCloseButtonProps,
isVisible,
domRef,
getBaseProps,
handleClose,
color,
onClose,
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const baseWrapper = useMemo(() => { | |
| return isVisible ? ( | |
| <div ref={domRef} {...getBaseProps()}> | |
| {alertIcon} | |
| {mainWrapper} | |
| {(isClosable || onClose) && ( | |
| <button onClick={handleClose} {...getCloseButtonProps()}> | |
| <CloseIcon {...getCloseIconProps()} height={20} width={20} /> | |
| </button> | |
| )} | |
| </div> | |
| ) : null; | |
| }, [ | |
| mainWrapper, | |
| isClosable, | |
| getCloseButtonProps, | |
| isVisible, | |
| domRef, | |
| getBaseProps, | |
| handleClose, | |
| color, | |
| onClose, | |
| ]); | |
| const baseWrapper = useMemo(() => { | |
| return isVisible ? ( | |
| <div ref={domRef} {...getBaseProps()}> | |
| {alertIcon} | |
| {mainWrapper} | |
| {(isClosable || onClose) && ( | |
| <button onClick={handleClose} {...getCloseButtonProps()}> | |
| <CloseIcon {...getCloseIconProps()} height={20} width={20} /> | |
| </button> | |
| )} | |
| </div> | |
| ) : null; | |
| }, [ | |
| alertIcon, | |
| getCloseIconProps, | |
| mainWrapper, | |
| isClosable, | |
| getCloseButtonProps, | |
| isVisible, | |
| domRef, | |
| getBaseProps, | |
| handleClose, | |
| color, | |
| onClose, | |
| ]); |
| const alertIcon = useMemo(() => { | ||
| switch (color) { | ||
| case "primary": | ||
| return <InfoCircleIcon {...getAlertIconProps()} />; | ||
| case "secondary": | ||
| return <InfoCircleIcon {...getAlertIconProps()} />; | ||
| case "success": | ||
| return <SuccessIcon {...getAlertIconProps()} />; | ||
| case "warning": | ||
| return <WarningIcon {...getAlertIconProps()} />; | ||
| case "danger": | ||
| return <DangerIcon {...getAlertIconProps()} />; | ||
| default: | ||
| return <InfoCircleIcon {...getAlertIconProps()} />; | ||
| } | ||
| }, [color, getAlertIconProps]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor alertIcon to reduce duplication
The switch statement for alertIcon repeats the same icon for multiple cases. Consider refactoring to improve maintainability.
You can map the colors to icons and simplify the code:
- const alertIcon = useMemo(() => {
- switch (color) {
- case "primary":
- return <InfoCircleIcon {...getAlertIconProps()} />;
- case "secondary":
- return <InfoCircleIcon {...getAlertIconProps()} />;
- case "success":
- return <SuccessIcon {...getAlertIconProps()} />;
- case "warning":
- return <WarningIcon {...getAlertIconProps()} />;
- case "danger":
- return <DangerIcon {...getAlertIconProps()} />;
- default:
- return <InfoCircleIcon {...getAlertIconProps()} />;
- }
- }, [color, getAlertIconProps]);
+ const iconMap = {
+ primary: InfoCircleIcon,
+ secondary: InfoCircleIcon,
+ success: SuccessIcon,
+ warning: WarningIcon,
+ danger: DangerIcon,
+ };
+
+ const IconComponent = iconMap[color] || InfoCircleIcon;
+
+ const alertIcon = useMemo(() => {
+ return <IconComponent {...getAlertIconProps()} />;
+ }, [color, getAlertIconProps]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const alertIcon = useMemo(() => { | |
| switch (color) { | |
| case "primary": | |
| return <InfoCircleIcon {...getAlertIconProps()} />; | |
| case "secondary": | |
| return <InfoCircleIcon {...getAlertIconProps()} />; | |
| case "success": | |
| return <SuccessIcon {...getAlertIconProps()} />; | |
| case "warning": | |
| return <WarningIcon {...getAlertIconProps()} />; | |
| case "danger": | |
| return <DangerIcon {...getAlertIconProps()} />; | |
| default: | |
| return <InfoCircleIcon {...getAlertIconProps()} />; | |
| } | |
| }, [color, getAlertIconProps]); | |
| const iconMap = { | |
| primary: InfoCircleIcon, | |
| secondary: InfoCircleIcon, | |
| success: SuccessIcon, | |
| warning: WarningIcon, | |
| danger: DangerIcon, | |
| }; | |
| const IconComponent = iconMap[color] || InfoCircleIcon; | |
| const alertIcon = useMemo(() => { | |
| return <IconComponent {...getAlertIconProps()} />; | |
| }, [color, getAlertIconProps]); |
| const baseWrapper = useMemo(() => { | ||
| return isVisible ? ( | ||
| <div ref={domRef} {...getBaseProps()}> | ||
| <InfoCircleIcon {getAlertIconProps()} /> |
There was a problem hiding this comment.
Fix missing spread operator in <InfoCircleIcon> props
The component <InfoCircleIcon> is used without the spread operator for its props. To correctly pass down the props returned by getAlertIconProps(), you should use the spread operator ....
Apply this diff to fix the issue:
-<InfoCircleIcon {getAlertIconProps()} />
+<InfoCircleIcon {...getAlertIconProps()} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <InfoCircleIcon {getAlertIconProps()} /> | |
| <InfoCircleIcon {...getAlertIconProps()} /> |
| const baseWrapper = useMemo(() => { | ||
| return isVisible ? ( | ||
| <div ref={domRef} {...getBaseProps()}> | ||
| <InfoCircleIcon {getAlertIconProps()} /> | ||
| {mainWrapper} | ||
| {(isClosable || onClose) && ( | ||
| <button onClick={handleClose} {...getCloseButtonProps()}> | ||
| <CloseIcon {...getCloseIconProps()}/> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ) : null; | ||
| }, [ | ||
| mainWrapper, | ||
| isClosable, | ||
| getCloseButtonProps, | ||
| isVisible, | ||
| domRef, | ||
| getBaseProps, | ||
| handleClose, | ||
| color, | ||
| onClose | ||
| ]); |
There was a problem hiding this comment.
Add missing dependencies in the useMemo hook
The useMemo hook relies on getAlertIconProps and getCloseIconProps, which are used within the memoized function but are not included in the dependency array. To ensure the memoized value updates correctly when these dependencies change, they should be added to the dependency array.
Apply this diff to include the missing dependencies:
}, [
mainWrapper,
isClosable,
getCloseButtonProps,
isVisible,
domRef,
getBaseProps,
handleClose,
color,
onClose,
+ getAlertIconProps,
+ getCloseIconProps,
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const baseWrapper = useMemo(() => { | |
| return isVisible ? ( | |
| <div ref={domRef} {...getBaseProps()}> | |
| <InfoCircleIcon {getAlertIconProps()} /> | |
| {mainWrapper} | |
| {(isClosable || onClose) && ( | |
| <button onClick={handleClose} {...getCloseButtonProps()}> | |
| <CloseIcon {...getCloseIconProps()}/> | |
| </button> | |
| )} | |
| </div> | |
| ) : null; | |
| }, [ | |
| mainWrapper, | |
| isClosable, | |
| getCloseButtonProps, | |
| isVisible, | |
| domRef, | |
| getBaseProps, | |
| handleClose, | |
| color, | |
| onClose | |
| ]); | |
| const baseWrapper = useMemo(() => { | |
| return isVisible ? ( | |
| <div ref={domRef} {...getBaseProps()}> | |
| <InfoCircleIcon {getAlertIconProps()} /> | |
| {mainWrapper} | |
| {(isClosable || onClose) && ( | |
| <button onClick={handleClose} {...getCloseButtonProps()}> | |
| <CloseIcon {...getCloseIconProps()}/> | |
| </button> | |
| )} | |
| </div> | |
| ) : null; | |
| }, [ | |
| mainWrapper, | |
| isClosable, | |
| getCloseButtonProps, | |
| isVisible, | |
| domRef, | |
| getBaseProps, | |
| handleClose, | |
| color, | |
| onClose, | |
| getAlertIconProps, | |
| getCloseIconProps, | |
| ]); |
| const InfoCircleIcon = `export const InfoCircleIcon = (props) => ( | ||
| <svg | ||
| fill="none" | ||
| height="24" | ||
| viewBox="0 0 24 24" | ||
| width="24" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| {...props} | ||
| > | ||
| <path | ||
| d="M12 22C17.51 22 22 17.51 22 12C22 6.49 17.51 2 12 2C6.49 2 2 6.49 2 12C2 17.51 6.49 22 12 22ZM12.75 16C12.75 16.41 12.41 16.75 12 16.75C11.59 16.75 11.25 16.41 11.25 16L11.25 11C11.25 10.59 11.59 10.25 12 10.25C12.41 10.25 12.75 10.59 12.75 11L12.75 16ZM11.08 7.62C11.13 7.49 11.2 7.39 11.29 7.29C11.39 7.2 11.5 7.13 11.62 7.08C11.74 7.03 11.87 7 12 7C12.13 7 12.26 7.03 12.38 7.08C12.5 7.13 12.61 7.2 12.71 7.29C12.8 7.39 12.87 7.49 12.92 7.62C12.97 7.74 13 7.87 13 8C13 8.13 12.97 8.26 12.92 8.38C12.87 8.5 12.8 8.61 12.71 8.71C12.61 8.8 12.5 8.87 12.38 8.92C12.14 9.02 11.86 9.02 11.62 8.92C11.5 8.87 11.39 8.8 11.29 8.71C11.2 8.61 11.13 8.5 11.08 8.38C11.03 8.26 11 8.13 11 8C11 7.87 11.03 7.74 11.08 7.62Z" | ||
| /> | ||
| </svg> | ||
| );`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor InfoCircleIcon to a proper React component
The InfoCircleIcon is currently defined as a string literal containing JSX code. This approach may lead to issues with syntax highlighting, code analysis, and potential security risks if used improperly.
Consider refactoring InfoCircleIcon to a proper React component in a separate file:
- Create a new file
InfoCircleIcon.tsx. - Define the component as a proper React functional component.
- Export the component as the default export.
Example:
import React from 'react';
const InfoCircleIcon: React.FC<React.SVGProps<SVGSVGElement>> = (props) => (
<svg
fill="none"
height="24"
viewBox="0 0 24 24"
width="24"
xmlns="http://www.w3.org/2000/svg"
{...props}
>
<path d="M12 22C17.51 22 22 17.51 22 12C22 6.49 17.51 2 12 2C6.49 2 2 6.49 2 12C2 17.51 6.49 22 12 22ZM12.75 16C12.75 16.41 12.41 16.75 12 16.75C11.59 16.75 11.25 16.41 11.25 16L11.25 11C11.25 10.59 11.59 10.25 12 10.25C12.41 10.25 12.75 10.59 12.75 11L12.75 16ZM11.08 7.62C11.13 7.49 11.2 7.39 11.29 7.29C11.39 7.2 11.5 7.13 11.62 7.08C11.74 7.03 11.87 7 12 7C12.13 7 12.26 7.03 12.38 7.08C12.5 7.13 12.61 7.2 12.71 7.29C12.8 7.39 12.87 7.49 12.92 7.62C12.97 7.74 13 7.87 13 8C13 8.13 12.97 8.26 12.92 8.38C12.87 8.5 12.8 8.61 12.71 8.71C12.61 8.8 12.5 8.87 12.38 8.92C12.14 9.02 11.86 9.02 11.62 8.92C11.5 8.87 11.39 8.8 11.29 8.71C11.2 8.61 11.13 8.5 11.08 8.38C11.03 8.26 11 8.13 11 8C11 7.87 11.03 7.74 11.08 7.62Z" />
</svg>
);
export default InfoCircleIcon;This change will improve code maintainability and reduce potential issues.
| const CloseIcon = `export const CloseIcon = (props) => ( | ||
| <svg | ||
| aria-hidden="true" | ||
| fill="none" | ||
| focusable="false" | ||
| height="1em" | ||
| role="presentation" | ||
| stroke="currentColor" | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| viewBox="0 0 24 24" | ||
| width="1em" | ||
| {...props} | ||
| > | ||
| <path d="M18 6L6 18M6 6l12 12" /> | ||
| </svg> | ||
| );`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor CloseIcon to a proper React component
The CloseIcon is currently defined as a string literal containing JSX code. This approach may lead to issues with syntax highlighting, code analysis, and potential security risks if used improperly.
Consider refactoring CloseIcon to a proper React component in a separate file:
- Create a new file
CloseIcon.tsx. - Define the component as a proper React functional component.
- Export the component as the default export.
Example:
import React from 'react';
const CloseIcon: React.FC<React.SVGProps<SVGSVGElement>> = (props) => (
<svg
aria-hidden="true"
fill="none"
focusable="false"
height="1em"
role="presentation"
stroke="currentColor"
strokeLinecap="round"
strokeLinejoin="round"
strokeWidth={2}
viewBox="0 0 24 24"
width="1em"
{...props}
>
<path d="M18 6L6 18M6 6l12 12" />
</svg>
);
export default CloseIcon;This change will improve code maintainability and reduce potential issues.
| const App = `import React, {forwardRef, useMemo} from "react"; | ||
| import {useAlert} from "@nextui-org/react"; | ||
| import {InfoCircleIcon} from "./InfoCircleIcon"; | ||
| import {CloseIcon} from "./CloseIcon" | ||
| const styles = { | ||
| base: [ | ||
| "bg-slate-100", | ||
| "border", | ||
| "shadow", | ||
| "hover:bg-slate-200", | ||
| "focus-within:!bg-slate-100", | ||
| "dark:bg-slate-900", | ||
| "dark:hover:bg-slate-800", | ||
| "dark:border-slate-800", | ||
| "dark:focus-within:!bg-slate-900", | ||
| "cursor-pointer" | ||
| ], | ||
| title: [ | ||
| "text-base", | ||
| "text-slate-500", | ||
| "font-bold" | ||
| ], | ||
| description: [ | ||
| "text-base", | ||
| "text-slate-500", | ||
| ], | ||
| } | ||
| const MyAlert = forwardRef((props, ref) => { | ||
| const { | ||
| title, | ||
| description, | ||
| isClosable, | ||
| domRef, | ||
| handleClose, | ||
| getBaseProps, | ||
| getMainWrapperProps, | ||
| getDescriptionProps, | ||
| getTitleProps, | ||
| getCloseButtonProps, | ||
| color, | ||
| isVisible, | ||
| onClose, | ||
| getCloseIconProps, | ||
| getAlertIconProps, | ||
| } = useAlert({ | ||
| ...props, | ||
| ref, | ||
| // this is just for the example, the props bellow should be passed by the parent component | ||
| title: "Email Sent!!", | ||
| description: "You will get a reply soon", | ||
| // custom styles | ||
| classNames: { | ||
| ...styles, | ||
| }, | ||
| }); | ||
| const mainWrapper = useMemo(() => { | ||
| return ( | ||
| <div {...getMainWrapperProps()}> | ||
| {title && <div {...getTitleProps()}>{title}</div>} | ||
| <div {...getDescriptionProps()}>{description}</div> | ||
| </div> | ||
| ); | ||
| }, [title, description, getMainWrapperProps, getTitleProps, getDescriptionProps]); | ||
| const baseWrapper = useMemo(() => { | ||
| return isVisible ? ( | ||
| <div ref={domRef} {...getBaseProps()}> | ||
| <InfoCircleIcon {...getAlertIconProps()} /> | ||
| {mainWrapper} | ||
| {(isClosable || onClose) && ( | ||
| <button onClick={handleClose} {...getCloseButtonProps()}> | ||
| <CloseIcon {...getCloseIconProps()}/> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ) : null; | ||
| }, [ | ||
| mainWrapper, | ||
| isClosable, | ||
| getCloseButtonProps, | ||
| isVisible, | ||
| domRef, | ||
| getBaseProps, | ||
| handleClose, | ||
| color, | ||
| onClose, | ||
| getAlertIconProps, | ||
| getCloseIconProps, | ||
| ]); | ||
| return <>{baseWrapper}</>; | ||
| }); | ||
| MyAlert.displayName = "MyAlert"; | ||
| export default MyAlert;`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor MyAlert to a proper React component file
The MyAlert component is currently defined as a string literal, which can lead to various issues including difficulty in maintenance, lack of proper syntax highlighting, and potential security risks.
Consider refactoring MyAlert into a separate React component file:
- Create a new file
MyAlert.tsx. - Move the component code into this file, removing the string literal wrapper.
- Properly import the required dependencies at the top of the file.
Example:
import React, { forwardRef, useMemo } from "react";
import { useAlert } from "@nextui-org/react";
import InfoCircleIcon from "./InfoCircleIcon";
import CloseIcon from "./CloseIcon";
// ... (rest of the component code)
export default MyAlert;This change will significantly improve code maintainability and reduce potential issues.
Remove hardcoded values for title and description
The component currently uses hardcoded values for the title and description. This limits the reusability of the component.
Remove the following lines:
// this is just for the example, the props bellow should be passed by the parent component
title: "Email Sent!!",
description: "You will get a reply soon",Allow these props to be passed from the parent component instead.
Add missing dependencies to useMemo hook
The useMemo hook for baseWrapper is missing some dependencies that are used within it. To ensure the memoized value updates correctly when these dependencies change, add getAlertIconProps and getCloseIconProps to the dependency array:
}, [
mainWrapper,
isClosable,
getCloseButtonProps,
isVisible,
domRef,
getBaseProps,
handleClose,
color,
onClose,
getAlertIconProps,
getCloseIconProps,
]);This will prevent potential bugs due to stale values.
| const react = { | ||
| "/App.jsx": App, | ||
| "/InfoCircleIcon": InfoCircleIcon, | ||
| "/CloseIcon": CloseIcon, | ||
| }; | ||
|
|
||
| export default { | ||
| ...react, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify and standardize the export structure
The current export structure, where React components are defined as string literals and exported through an object, is unconventional and can lead to various issues. This approach makes it difficult to maintain, debug, and use these components effectively.
Consider restructuring the exports as follows:
-
After refactoring each component into its own file as suggested in previous comments, update this file to import and export the components directly.
-
Remove the
reactobject and the complex export structure.
Example of a simplified export structure:
import InfoCircleIcon from './InfoCircleIcon';
import CloseIcon from './CloseIcon';
import MyAlert from './MyAlert';
export {
InfoCircleIcon,
CloseIcon,
MyAlert,
};This restructuring will significantly improve code maintainability, readability, and adherence to React best practices. It will also make it easier for other parts of the application to import and use these components.
|
Okay, will look into that |
📝 Description
Summary by CodeRabbit
New Features
Documentation
Tests