shadcn skeleton #721
shadcn skeleton #721
Conversation
airhorns
force-pushed
the
shadcn-skeleton
branch
2 times, most recently
from
91be6af to
a0ee46d
Compare
airhorns
force-pushed
the
shadcn-skeleton
branch
from
a0ee46d to
11ac63f
Compare
| import type { ShadcnElements } from "../elements.js"; | ||
|
|
||
| // should we move this to a shared location? | ||
| import { cn } from "../../../../spec/auto/shadcn-defaults/utils.js"; |
There was a problem hiding this comment.
I think the move would be to expect this to be passed in as an element that you can then get from the context, as users are likely gonna have their own version that they use in their shadcn components, and we wnat to use the same one. generally speaking, any import from the spec dir in the src dir is busted
There was a problem hiding this comment.
@airhorns This import was to merge the styles that came with the users component and add an error state if the input had an error. Thanks for the feedback
There was a problem hiding this comment.
Right, I understant we need the utility, but I am saying we can't import it ourselves from the version in spec. I think this utility needs to be provided to us as one of the elements, and then referred to from the elements at runtime. The user will need their own version of this utility in their app, and will use it in all their shadcn components, we should re-use theirs, not import from our own. In general, any code in src importing from spec is an antipattern as well, if we intend to ship the code at runtime it needs to be in src. But, that's why I put the default components in spec -- we don't intend to ship any of that at runtime, its just a copy of shadcn we use for testing.
| <Elements extends ShadcnElements>({ Form, Input, Button, Alert, Skeleton, AlertTitle, AlertDescription, Label, Checkbox }: Elements) => | ||
| <GivenOptions extends OptionsType, SchemaT, ActionFunc extends ActionFunction<GivenOptions, any, any, SchemaT, any>>( | ||
| props: AutoFormProps<GivenOptions, SchemaT, ActionFunc> & ComponentProps<typeof Form> | ||
| ) => { |
There was a problem hiding this comment.
just realizing that we should probably use named functions instead of anonymous functions in these factories here so that stack traces and the React dev tools can use the right name for the components in development
| import type { ShadcnElements } from "../elements.js"; | ||
|
|
||
| // should we move this to a shared location? | ||
| import { cn } from "../../../../spec/auto/shadcn-defaults/utils.js"; |
There was a problem hiding this comment.
Right, I understant we need the utility, but I am saying we can't import it ourselves from the version in spec. I think this utility needs to be provided to us as one of the elements, and then referred to from the elements at runtime. The user will need their own version of this utility in their app, and will use it in all their shadcn components, we should re-use theirs, not import from our own. In general, any code in src importing from spec is an antipattern as well, if we intend to ship the code at runtime it needs to be in src. But, that's why I put the default components in spec -- we don't intend to ship any of that at runtime, its just a copy of shadcn we use for testing.
| */ | ||
| if (name === "Shadcn") { | ||
| return; | ||
| } |
There was a problem hiding this comment.
tiny nit: I think this is going to be easy to forget to fix later cause its going to show up as a green test when we look at the list of tests. can you file a github issue to ensure we come back to re-enable this?
| ); | ||
| AlertDescription.displayName = "AlertDescription"; | ||
|
|
||
| export { Alert, AlertDescription, AlertTitle }; |
There was a problem hiding this comment.
I highly recommend configuring your editor to use the eslint / prettier config that is set up in the repo already so that all the code you write matches the formatting rules we have set already and you dont have to waste time reformatting stuff manually 👍 see the root level package scripts for the commands to run to fix lint errors automatically too, like pnpm lint:fix
| const AutoInput = (props: { suiteName: string; field: string; label?: string }) => { | ||
| if (props.suiteName === "Polaris") { | ||
| return <PolarisAutoInput {...props} />; | ||
| } | ||
|
|
||
| if (props.suiteName === "Shadcn") { | ||
| const ShadcnAutoInput = makeShadcnAutoInput({ Input: Input, Label: Label, Checkbox: Checkbox, Button: Button }); |
There was a problem hiding this comment.
nit: can you move this outside of this function? Otherwise, it's going to re-create the input component every render and for every test, which is kinda slow, and also not how production uses of this stuff will work so I think it will test it a bit differently. the factory functions should generally be invoked in the module scope
| }); | ||
| }; | ||
|
|
||
| // TODO: move to a shared location |
There was a problem hiding this comment.
👍 definitely -- as well as the stuff above with the step if that is shared with polaris
|
|
||
| const [isEditing, setIsEditing] = useState(!findBy); | ||
|
|
||
| const startEditing = () => { |
There was a problem hiding this comment.
should probably be a useCallback
packages/react/package.json
Outdated
| @@ -44,6 +49,7 @@ | |||
| "dependencies": { | |||
| "@0no-co/graphql.web": "^1.0.4", | |||
| "@gadgetinc/api-client-core": "^0.15.38", | |||
| "@gadgetinc/react": "workspace:^", | |||
There was a problem hiding this comment.
This is gonna break stuff -- the package can't depend on itself, and can't depend on a workspace version of the package as that won't be available on npm. imports within the package should be relative to avoid referring to it by name
WIP implementation of AutoButton for shadcn, showing the factory pattern and example test layout