chore(web_common): move more shared components#5320
Conversation
benfdking
left a comment
benfdking
left a comment
There was a problem hiding this comment.
I am quite hesitant about this PR because, like others in this vein, we are preempting abstraction.
I think it combines things that make for a perfect storm of pain:
- Visual code is incredibly hard to test and so very hard to alter
- The code isn't inherently composable
- It feels like we've pre-emptively abstracted and tied together
- It's more to learn that people just don't
Visual Code is incredibly hard to test
I think this is self-explanatory, but testing any visual code is inherently difficult. When you build a library that is going to be used by downstream applications and it's hard to test those, it is incredibly hard to change this library and so people inherently don't. Hence why you want to keep it as small as possible.
Pre-emptive abstraction
To me, this feels like we are building a library; we don't quite know whether we will use all the components yet (by that I mean, are you going to use it in two places tomorrow), and because of that, we make our code rigid and more likely than not we have made bad and unnecessary abstractions. This combines with how difficult it's going to be test it it, I w
https://ozzs.dev/premature-vs-unnecessary-abstraction
"Less is more" and this to me doesn't inherently do that.
The code isn't inherently composable
I think there are some elements that could make this way easier to compose
- Generics
- Dependency injection of fuse.js
- Examples of compositions rather than components that do it for you
Hard to learn
One of the lessons I have learned over and over again is that people are very slow to learn new things. I know Tailwind, but I don't know this library. So as a developer, I will jump to Tailwind classes most always before jumping to these classes. That instinct is very hard to beat. I think by doing less, being more precise, you are more likely to avoid that.
| 'use client' | ||
|
|
||
| import { type DialogProps } from '@radix-ui/react-dialog' | ||
| import { Command as CommandPrimitive } from 'cmdk' |
There was a problem hiding this comment.
I don't think we should have anything in an extra bucket. It's like utils, it's, to me, just a bin for things to get lost, especially for a library this feels like a dangerous precedent.
| export const Description = React.forwardRef< | ||
| HTMLDivElement, | ||
| React.HTMLAttributes<HTMLDivElement> | ||
| >( | ||
| ( | ||
| { | ||
| children, | ||
| className, | ||
| ...props | ||
| }: { | ||
| children?: React.ReactNode | ||
| className?: string | ||
| }, | ||
| ref, | ||
| ) => { | ||
| return ( | ||
| <div | ||
| ref={ref} | ||
| className={cn('text-typography-description text-sm', className)} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </div> | ||
| ) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
I'm not sure if this is copied over, but this feels like the most egregious example of premature optimization.
This component adds a huge amount of complexity/code for just replacing text-typography-description text-sm. The overhead to learn this component feels quite unnecessary.
| import type { HeadlineLevel } from '@/types' | ||
| import { cn } from '@/utils' | ||
|
|
||
| export function Headline({ |
There was a problem hiding this comment.
This feels similar to me, this is just an h-tag which shouldn't be optional + two classes. Why do we need a component for that?
| export interface TextBlockProps extends React.HTMLAttributes<HTMLDivElement> { | ||
| level?: HeadlineLevel | ||
| headline?: string | ||
| tagline?: string | ||
| info?: string | ||
| } |
There was a problem hiding this comment.
nit: To me I would ask what is wrong with removing this and just giving
<Headline> ... </Headline>
<Text> ... </Text>
| afterInput, | ||
| }: { | ||
| items: TItem[] | ||
| filterOptions?: IFuseOptions<any> |
There was a problem hiding this comment.
Injecting the whole search function feels to me like a really simple way to avoid this.
| import MessageContainer from '../MessageContainer/MessageContainer' | ||
|
|
||
| export function FilterableList< | ||
| TItem extends Record<string, any> = Record<string, any>, |
There was a problem hiding this comment.
Our libraries shouldn't have any, we should really ban any, this should definitely be a generic.
mykalmax
force-pushed
the
mykalmax/web_common-more-components
branch
from
5e12255 to
aed4bcd
Compare
3 participants
No description provided.