Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: extend alert interface with div props #125

Merged
merged 1 commit into from
May 31, 2024
Merged

feat: extend alert interface with div props #125

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/components/alert/alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ const iconNames: Record<AlertIntent, React.ElementType> = {
danger: ErrorIcon,
};

export interface AlertProps {
export interface AlertProps extends React.ComponentPropsWithoutRef<"div"> {
title: string;
intent: AlertIntent;
children?: React.ReactNode;
}

export const Alert = ({ title, children, intent }: AlertProps) => {
export const Alert = ({ title, children, intent, ...props }: AlertProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've seen another component that was doing this, and it was pretty hard to use. The autocomplete had a huge list of options, and it was hard to find the ones that are "intended props to control this" and which were "optional prop drilling for parent element". I'm not sure what's the intended case here, either.

What do you think about:

  1. Explicitly typing only the things we want to modify, like classnames or some listener/callbacks. We would inject tailwind classes for all styling.

  2. Having an explicit prop to pass props to the parent div. This would keep the usage clean

interface DemoCompProps {
   title: string; 
   // ...
   topLevelProps: React.ComponentPropsWithoutRef<"div">;
}

below we would deestructure topLevelProps instead of whatever falls into the ... props } thing

Copy link
Contributor Author

@mnlfischer mnlfischer May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your feedback. In general, I completely agree with you that we only pass on props that we really want to make available and make it easy to access.

  1. we always proceed in this way until we reach a point where we need the native properties. I think this approach is good because we start with the smallest and then build up from there. When we need the native properties, I think it's more solid to pass the ones defined by React completely, which will create the same behavior as the native element and complement it. For example, the data-attribute properties are well defined, which we either have to define completely ourselves anyway or create utility functions, which increases the complexity and we continue to increase it with each additional similar property.
    I agree that it's not beneficial for autocompletion, however you can always jump into the interface definition or look it up in the storybook.

  2. the problem with child elements is unfortunately not solved nicely and we try to split the component so small that the passing of the properties only reaches the component that has been implemented and no child elements are affected. Many components have a very granular division and here we could consider extending the component with a title component to make it clear that the properties only apply to the explicit component.
    I consider the distribution of properties to certain child elements to be more complex, as in this example it is relatively simple, but there are components that have significantly more child elements if they have not been defined granularly enough. The problem is then rather the definition of the component that should be solved.

Idea
Extract the title into Alert.title component and create a real compound component but this is also a really small component and maybe not worth it regarding the effort vs. solving the issue that can be solved by a click into the interface definition. Also the icon could be valid to extract but the design is fixed by putting it to the left and this would be open it a bit too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

Point 1 was not about duplicating props, but expose them in a controlled manner. For example, picking callbacks from DOMAttributes for callbacks, extending only AriaAttributes, etc.
But the main point is pretty fair, the type definition should be enough to see what's custom and what's extended from React.

If I understand correctly, your problem with 2 is that in some components we need to inject things to several children, so we should create subcomponents instead of named props. I think this makes sense as a better alternative, but does not discount naming the injected props to make them more ergonomic.

We could have something like injectedProps, which we only allow one of, so if you need to expose children there, you need to create a subcomponent. The extra complexity is minimal, and it's a bit cleaner/more explicit, but I get that that's going away from the spirit of exposing a "modified html element" you can interact with normally. This seems to be a matter of opinion, though.

I think I raised this point because when moving away from abusix-ui, I've seen a bunch of custom props, misused ...props and stuff like that. At some point, if you expose too much and the consumers are not careful, the component library loses its point, and it makes it hard to migrate.

I'm still unsure about going deeper into this pattern, but I'll approve to not block the thing, specially considering the pattern already exists in the codebase. Thanks for the analysis here!

Copy link
Contributor Author

@mnlfischer mnlfischer May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I will note that for a deeper investigation if we can find a better solution or define a general way of how we want to create component interfaces. Your feedback is valuable and this topic is not easy and until now I did not find any solution without a drawback. Lets have a look into public libs and get a feeling about if we are on a good way! Thanks @rodelta!

const Icon = iconNames[intent];

return (
Expand All @@ -41,6 +41,7 @@ export const Alert = ({ title, children, intent }: AlertProps) => {
"flex flex-row gap-4 rounded-lg border px-4 py-3 text-neutral-800",
alertVariants[intent]
)}
{...props}
>
<Icon className={classNames("h-4 w-4 flex-shrink-0", iconVariants[intent])} />
<div className="flex-grow">
Expand Down
Loading