-
Notifications
You must be signed in to change notification settings - Fork 2.3k
DaisyUI v5 + Tailwind v4 + RJSF v6 #4498
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.
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
Conversation
@heath-freenome Can you provide guidance to @inchoate about your new grid templating system in v6 and what other major changes (if any) he might expect to impact this PR? Would it make sense for us to prioritize merging this and #4497 to v6 before additional templating changes? |
@nickgros @inchoate I've completed all of the template changes in V6 already. It seems like there needs to be an implementation of |
<div className='card-body p-4'> | ||
<div className='array-field-item-content mb-2'>{children}</div> | ||
<div className='card-actions justify-end'> | ||
{hasMoveUp && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will want to use the ArrayFieldItemButtonsTemplate
in place of the buttons here. Although your implementation has additional behavior to the default implementation so maybe it is not necessary.
F extends FormContextType = any | ||
>(): ComponentType<FormProps<T, S, F>> { | ||
const theme = generateTheme<T, S, F>(); | ||
console.log('Generated theme:', theme); // Debug what templates are available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all of the console.log()
statements before committing
console.log('DaisyUI ArrayFieldTitleTemplate'); | ||
|
||
props; | ||
return <div>ArrayFieldTitleTemplate</div>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why you implemented this since it does nothing to display the TitleFieldTemplate
@@ -0,0 +1,2 @@ | |||
export { default } from './BaseInputTemplate'; | |||
export * from './BaseInputTemplate'; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export * from './BaseInputTemplate'; | |
export * from './BaseInputTemplate'; | |
@@ -0,0 +1,2 @@ | |||
export { default } from './DescriptionField'; | |||
export * from './DescriptionField'; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export * from './DescriptionField'; | |
export * from './DescriptionField'; | |
packages/daisyui/src/types.d.ts
Outdated
declare module '*.css?inline' { | ||
const content: string; | ||
export default content; | ||
} No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
packages/daisyui/src/utils.ts
Outdated
// Object.keys(daisyProps).forEach((key) => { | ||
// /** | ||
// * Leveraging `shouldForwardProp` to remove props | ||
// * | ||
// * This is a utility function that's used in `@daisy-ui/react`'s factory function. | ||
// * Normally, it prevents DaisyProps from being passed to the DOM. | ||
// * In this case we just want to delete the unknown props. So we flip the boolean. | ||
// */ | ||
// if (shouldForwardProp(key)) { | ||
// delete (daisyProps as any)[key]; | ||
// } | ||
// }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code meant to be commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
// | ||
// Predefined DayPicker styles using DaisyUI classes | ||
// | ||
const dayPickerStyles: { classNames: Partial<ClassNames>; modifiers: Partial<ModifiersClassNames> } = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to DRY up this code into the DateTimeWidget
with a flag to only show Date?
import Ajv2019 from 'ajv/dist/2019.js'; | ||
import Ajv2020 from 'ajv/dist/2020.js'; | ||
|
||
import Layout from './layout'; | ||
import Playground, { PlaygroundProps } from './components'; | ||
|
||
// @ts-expect-error todo: error TS2345: Argument of type 'Localize' is not assignable to parameter of type 'Localizer'. | ||
const esV8Validator = customizeValidator({}, localize_es); | ||
const AJV8_2019 = customizeValidator({ AjvClass: Ajv2019 }); | ||
const AJV8_2020 = customizeValidator({ AjvClass: Ajv2020 }); | ||
const AJV8_DISC = customizeValidator({ ajvOptionsOverrides: { discriminator: true } }); | ||
const AJV8_DATA_REF = customizeValidator({ ajvOptionsOverrides: { $data: true } }); | ||
|
||
const validators: PlaygroundProps['validators'] = { | ||
AJV8: v8Validator, | ||
'AJV8 $data reference': AJV8_DATA_REF, | ||
'AJV8 (discriminator)': AJV8_DISC, | ||
AJV8_es: esV8Validator, | ||
AJV8_2019, | ||
AJV8_2020, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please restore all of these
} | ||
] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
@heath-freenome thanks for your comments. I believe I addressed everything you brought up. Clearly my commit hooks were not set up correctly and, yet they still refuse to run. I also added: |
@inchoate I don't see an implementation of the |
FieldErrorTemplate, | ||
FieldHelpTemplate, | ||
FieldTemplate, | ||
ObjectFieldTemplate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are missing a GridTemplate
implementation.
Sorry. Didn't see you make a comment about that. I'll just read the core implementation and go from there unless I hear otherwise. |
@nickgros / @heath-freenome I'm kinda' flailing on the GridTemplate right now. How can I accurately test a different theme's implementation on the v6 branch? |
I'm working on a new Field that will use it. Give me a few days to get it committed |
Awesome. Thanks.
…On Wed, Feb 19, 2025, 12:16 PM Heath C ***@***.***> wrote:
I'm working on a new Field that will use it. Give me a few days to get it
committed
—
Reply to this email directly, view it on GitHub
<#4498 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAH34OUF3YWNPHYPFMIDJF32QTC6JAVCNFSM6AAAAABXE7EPRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRZGQYTSNJQGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: heath-freenome]*heath-freenome* left a comment
(rjsf-team/react-jsonschema-form#4498)
<#4498 (comment)>
I'm working on a new Field that will use it. Give me a few days to get it
committed
—
Reply to this email directly, view it on GitHub
<#4498 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAH34OUF3YWNPHYPFMIDJF32QTC6JAVCNFSM6AAAAABXE7EPRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRZGQYTSNJQGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@inchoate I've merged a few PRs with updates and fixes from |
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e", | ||
"dependencies": { | ||
"tailwindcss": "^4.0.1" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this and do what it takes to have it work with standard npm install
commands.
"types": "src/index.ts", | ||
"scripts": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add these to improve ESM support, matching the changes made in 5.24.7
"types": "src/index.ts", | |
"scripts": { | |
"types": "src/index.ts", | |
"exports": { | |
".": { | |
"require": "./dist/index.js", | |
"import": "./lib/index.js", | |
"types": "./lib/index.d.ts" | |
}, | |
"./lib": { | |
"require": "./dist/index.js", | |
"import": "./lib/index.js", | |
"types": "./lib/index.d.ts" | |
}, | |
"./lib/*.js": { | |
"require": "./dist/*.js", | |
"import": "./lib/*.js", | |
"types": "./lib/*.d.ts" | |
}, | |
"./dist": { | |
"require": "./dist/index.js", | |
"import": "./lib/index.js", | |
"types": "./lib/index.d.ts" | |
}, | |
"./dist/*.js": { | |
"require": "./dist/*.js", | |
"import": "./lib/*.js", | |
"types": "./lib/*.d.ts" | |
} | |
}, | |
"scripts": { |
"main": "src/index.ts", | ||
"types": "src/index.ts", | ||
"scripts": { | ||
"build": "tsc -b", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve ESM support you will need to change this to:
"build": "tsc -b", | |
"build:ts": "tsc -b tsconfig.build.json && tsc-alias -p tsconfig.build.json", |
And add a tsconfig.build.json
that looks like:
{
"extends": "../../tsconfig.build.json",
"compilerOptions": {
"outDir": "./lib"
},
"files": [],
"references": [
{
"path": "./src"
}
],
"tsc-alias": {
"resolveFullPaths": true,
"verbose": true,
}
}
@@ -0,0 +1,2 @@ | |||
export { default } from './DaisyForm'; | |||
export * from './DaisyForm'; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export * from './DaisyForm'; | |
export * from './DaisyForm'; | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix the errors on this page, it seems like you need a tsconfig.json
in your packages/daisyui
that looks like:
{
"extends": "../../tsconfig.base.json",
"files": [],
"references": [
{
"path": "./src"
},
{
"path": "./test"
}
]
}
{canExpand<T, S, F>(schema, uiSchema, formData) && ( | ||
<div className='flex justify-end'> | ||
<AddButton | ||
className='btn btn-primary btn-sm' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add id={buttonId<T>(id, 'add')}
to match changes made in other themes in v6
disabled={disabled || readonly} | ||
/> | ||
{schema.additionalProperties && ( | ||
<button className='btn btn-danger ml-2' onClick={onDropPropertyClick(label)} disabled={disabled || readonly}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add id={buttonId<T>(id, 'remove')}
to match changes made in other themes in v6
{canAdd && ( | ||
<div className='flex flex-row justify-end'> | ||
<AddButton | ||
className='btn btn-primary' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add id={buttonId<T>(id, 'add')}
to match changes made in other themes in v6
@heath-freenome -- haven't forgotten this, I should be circling back to it shortly. Appreciate the patience. |
@heath-freenome I'm back at it. I'll be rebasing and adapting to the current state of https://github.com/rjsf-team/react-jsonschema-form/tree/rjsf-v6. Please let me know if I should be doing something else. |
@inchoate I noticed you now have two PRs? |
@heath-freenome yes -- I left this open for the conversation history. We can close it. Please see the other, which is quite better done. |
Reasons for making this change
[Please describe them here]
If this is related to existing tickets, include links to them as well. Use the syntax
fixes #[issue number]
(ex:fixes #123
).If your PR is non-trivial and you'd like to schedule a synchronous review, please add it to the weekly meeting agenda: https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.