chore(web_common): add more shared components#5243
Conversation
benfdking
left a comment
benfdking
left a comment
There was a problem hiding this comment.
I think this is fine, see my comments.
This does make me nervous, though. Looking at this, I would, for example, be nervous about including this in the vscode extension, it feels like pulling this in, signs me up to a lot of complexity and eventual tech debt that I have spent unnecessary hours fighting in the past.
| export interface ButtonProps | ||
| extends React.ButtonHTMLAttributes<HTMLButtonElement>, | ||
| VariantProps<typeof buttonVariants> { | ||
| asChild?: boolean | ||
| type?: ButtonType | ||
| } |
There was a problem hiding this comment.
I have been looking at this Button and I worry that the structure of it makes it quite difficult to figure out how it works. I think we can do better to make these components easier to consume for each other.
Here is my thought process. When you use a component, the way you want to understand it is:
- What are the props?
- How does it function?
This is why in past lives I have always structured a component file like.
// Imports
import type { ButtonType } from './help'
// Prop of component, named Props so you know it's the props for that component
export interface Props {}
// Component
export const Component = () => {}
// Anything else that makes up the Component
const InternalComponent = () => {}It's not a thing but figuring out how this Button component works requires quite a bit of jumping around files and while this is small, I think the complexity is pretty signficant to figure out for something so small.
| export const EnumButtonType = { | ||
| Button: 'button', | ||
| Submit: 'submit', | ||
| Reset: 'reset', | ||
| } as const |
There was a problem hiding this comment.
I am going to keep slowly pushing against enums 👅
https://bluepnume.medium.com/nine-terrible-ways-to-use-typescript-enums-and-one-good-way-f9c7ec68bf15 or even subtypes.
type ButtonType = "button" | "submit" | "reset"
verifying switches are exhaustive
and the usage with just to me is a nicer experience.
| variant: { | ||
| control: { type: 'select' }, | ||
| options: Object.values(EnumButtonVariant), | ||
| }, | ||
| size: { | ||
| control: { type: 'select' }, | ||
| options: Object.values(EnumSize), | ||
| }, | ||
| type: { | ||
| control: { type: 'select' }, | ||
| options: ['button', 'submit', 'reset'], | ||
| }, | ||
| disabled: { | ||
| control: 'boolean', | ||
| }, |
There was a problem hiding this comment.
Do we have to write these? We should be able to set it up so an enum can be inferred?
| }, | ||
| size: { | ||
| control: { type: 'select' }, | ||
| options: Object.values(EnumSize), | ||
| }, | ||
| disabled: { | ||
| control: 'boolean', | ||
| }, | ||
| title: { | ||
| control: 'text', | ||
| }, |
There was a problem hiding this comment.
This should all be inferred with a default?
| export interface ClipboardCopyProps extends Omit<ButtonProps, 'children'> { | ||
| text: string | ||
| delay?: number | ||
| children: (copied: boolean) => React.ReactNode | ||
| } | ||
|
|
||
| export const ClipboardCopy = React.forwardRef< | ||
| HTMLButtonElement, | ||
| ClipboardCopyProps |
There was a problem hiding this comment.
Nit: Would a shared hook not be a more flexible option for this?
There was a problem hiding this comment.
so we dont have a use case other than having button , (and i renamed component to be CopyButton) . once we have need to copy text from other interaction i can move to custom hook
web/common/src/utils/index.ts
Outdated
| export function isString(value: unknown): value is string { | ||
| return typeof value === 'string' | ||
| } | ||
| export function notString(value: unknown): value is typeof value { | ||
| return !isString(value) | ||
| } | ||
| export function isEmptyString(value: unknown): value is EmptyString { | ||
| return isString(value) && value.length === 0 | ||
| } | ||
| export function isNilOrEmptyString( | ||
| value: unknown, | ||
| ): value is Maybe<EmptyString> { | ||
| return isNil(value) || isEmptyString(value) | ||
| } | ||
| export function nonEmptyString(value: unknown): value is string { | ||
| return isString(value) && value.length > 0 | ||
| } |
There was a problem hiding this comment.
This scares me a little because it implies:
- that we pass around quite a lot of known.
- It ties code together which doesn't need to be tied together, to me this is an unnecessary, it means that for the most simple things like testing a string is empty
s.length === 0we know have tied our code to this very far reachingutilspart of the codebase.
An example of this is that if I want to move code around which previously had no tie to anything else because it's just checking whether a string is empty, I now need to think about this very big utils package which I really wouldn't want to do.
2 participants
No description provided.