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.

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

Add support for PrimeReact #2723

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Add support for PrimeReact #2723

wants to merge 17 commits into from

Conversation

dchenk
Copy link

@dchenk dchenk commented Feb 19, 2022

Reasons for making this change

This PR adds support for the PrimeReact library for the form theme. It's a popular library and is used by thousands of projects. The GitHub repo: https://github.com/primefaces/primereact

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@@ -48,6 +48,7 @@
"@rjsf/core": "^4.0.0",
"@rjsf/fluent-ui": "^4.0.0",
"@rjsf/material-ui": "^4.0.0",
"@rjsf/primereact": "https://gitpkg.now.sh/dchenk/react-jsonschema-form/packages/primereact?8a7eb51f2f92d691dd2901476c54dd88245fb9d3",
Copy link
Author

Choose a reason for hiding this comment

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

I added this to make sure that the playground works, and the version here can simply be updated upon the next release just like all the other @rjsf/* packages.

Copy link
Member

Choose a reason for hiding this comment

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

Why lock it down to a specific version?

# v4.1.0 (upcoming)

## @rjsf/primereact
- Add support for the PrimeReact component library.
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to bump this to v4.2.0

@@ -48,6 +48,7 @@
"@rjsf/core": "^4.0.0",
"@rjsf/fluent-ui": "^4.0.0",
"@rjsf/material-ui": "^4.0.0",
"@rjsf/primereact": "https://gitpkg.now.sh/dchenk/react-jsonschema-form/packages/primereact?2ade66860050bf49cfa51680a35802ce6afb63b9",
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to "^4.0.0"?

<!-- ABOUT THE PROJECT -->

## About The Project

Copy link
Member

Choose a reason for hiding this comment

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

There is an extra blank line here that can be removed

@@ -0,0 +1,54 @@
<svg width="62" height="71" viewBox="0 0 62 71" fill="none" xmlns="http://www.w3.org/2000/svg">
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to move this to the src directory?

required={required}
disabled={disabled}
readOnly={readonly}
value={value ? value : ""}
Copy link
Member

Choose a reason for hiding this comment

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

You can use value || "" instead

Comment on lines +103 to +116
onBlur &&
(() => {
onBlur(id, processValue(schema, value));
})
}
onFocus={
onFocus &&
(() => {
onFocus(id, processValue(schema, value));
})
}
onChange={(event) => {
onChange(processValue(schema, getValue(event)));
}}
Copy link
Member

Choose a reason for hiding this comment

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

Recommend moving these above as fat-arrow function like the other libraries are doing and just using the variable here

Comment on lines +128 to +142
onBlur={
onBlur &&
(() => {
onBlur(id, processValue(schema, value));
})
}
onFocus={
onFocus &&
(() => {
onFocus(id, processValue(schema, value));
})
}
onChange={(event) => {
onChange(processValue(schema, getValue(event)));
}}
Copy link
Member

Choose a reason for hiding this comment

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

Following the recommendation above you can use the same variables here

</label>
)}
{multiple ? (
<MultiSelect
Copy link
Member

Choose a reason for hiding this comment

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

Recommend doing something like:

const Selector = multiple ? MultiSelect : Dropdown;

then using `<Selector ... /> here since all the props are identical

@jacqueswho jacqueswho added enhancement core feature Is a feature request labels Apr 11, 2022
@happywang23
Copy link

@dchenk @heath-freenome Any updates to this? since we do really want to user rjsf within our project which is written in primereaact

@heath-freenome
Copy link
Member

@dchenk Due to the upcoming v5 release, the PrimeReact PR will need some work due to all of the breaking changes. I am willing to work with you to get PrimeReact in as a 5.x enhancement or even as part of the main release if it is ready in time

@dchenk
Copy link
Author

dchenk commented Jul 23, 2022

@dchenk Due to the upcoming v5 release, the PrimeReact PR will need some work due to all of the breaking changes. I am willing to work with you to get PrimeReact in as a 5.x enhancement or even as part of the main release if it is ready in time

@heath-freenome do you want to take over from here? I won't have capacity in the near future to see this through.

@heath-freenome
Copy link
Member

@dchenk I'm pretty busy just getting 5.0 ready. Once I have finished writing up the migration guide, you hopefully could make the necessary changes.

@heath-freenome
Copy link
Member

@dchenk Ok, v5 beta has landed. There are some new templates that will hopefully make things a little easier. See the 5.x migration guide as well as the expanded custom templates documentation

@heath-freenome heath-freenome added the v5 refactor Needs refactor due to v5 breaking changes label Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes core enhancement feature Is a feature request v5 refactor Needs refactor due to v5 breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants