-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: Add Bootstrap 4 Theme support #1938
feat: Add Bootstrap 4 Theme support #1938
Conversation
Co-authored-by: Jani Eväkallio <[email protected]>
Co-authored-by: Jani Eväkallio <[email protected]>
Co-authored-by: Jani Eväkallio <[email protected]>
…and support. refactored form component
…owship/react-jsonschema-form into MLH-Fellowship-react-bootstrap-4-final
Some comments after looking through the playground: I think the description field is too big here ("Simple" tab): Any idea why the "nested" example has cut off text? "nested" example -- can you make sure the buttons ("+" and "-") have a consistent size? "nested" example -- the "Done?" is supposed to be a checkbox, but it's rendering as a big title: "nested" example -- apparently "widgets" example -- the read-only fields should have the "simple" example -- whenever I type anything in a textbox, I get this error in the console: Validation - the validation doesn't seem to look quite like what default validation in bootstrap looks like. Can you use this to render inline validation errors instead: https://react-bootstrap.github.io/components/forms/#forms-validation-libraries Error List - any idea why the bottom left and bottom right corners aren't connected? Also, can you add a bit more margin below the error list (like how it looks like for the bootstrap 3 form)? "Widgets" tab - I am not able to click on "No" to change the value for the radio button. "Files" tab - can you change the file input to use Bootstrap's file input (https://getbootstrap.com/docs/4.0/components/forms/#file-browser)? "Alternatives" tab - I can't select anything in the multiselect widget: "Additional Properties" tab - can you implement the additional properties functionality here? "Nested" tab - can you make the help text grey with the |
…4-final Fix CI build
@epicfaace We have updated the tests and applied suggestions. |
@anikethsaha can you merge the latest master into this branch? That should fix the netlify issue. |
netlify build fixes
@epicfaace I did the merge, still failing. |
@epicfaace it seems the build is still failing. From the logs, it looks like it is simply running the let us know if anything else is required to fix from our end |
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.
Looks good! I wouldn't worry about the Netlify errors; it's probably a problem on their end.
A small minor changes, and this PR should be good to merge. We don't need the file input and additional properties features in order to merge this initial PR in, so I've made separate issues for them (#1962 and #1963). If you're able to work on them, that would be great!
|
||
const DescriptionField = ({ description }: Partial<FieldProps>) => { | ||
if (description) { | ||
return <div><h6 className="mb-5">{description}</h6></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.
We don't want descriptions to be in bold
return <div><h6 className="mb-5">{description}</h6></div>; | |
return <div><div className="mb-5">{description}</div></div>; |
{errors.map((error, i: number) => { | ||
return ( | ||
<ListGroup.Item key={i} className="border-0"> | ||
<small>{error.stack}</small> |
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.
Let's use regular-sized text for ErrorList
<small>{error.stack}</small> | |
<span>{error.stack}</span> |
import { ErrorListProps } from "@rjsf/core"; | ||
|
||
const ErrorList = ({ errors }: ErrorListProps) => ( | ||
<Card border="danger" className="mb-2"> |
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.
Need more margin between this and the form title
<Card border="danger" className="mb-2"> | |
<Card border="danger" className="mb-4"> |
@epicfaace I have updated the code with your changes. Let us know if there is anything else required. |
I'm super excited to try this out -- are you planning on updating the documentation so it references this theme? I'm not clear how to actually use it. It appears, based on the other themes, that I need to install @rjsf/bootstrap-4, which doesn't exist. Edit: Ok, I found the nested documentation for the package. The @rjsf/bootstrap-4 package appears to not be in the npm registry. |
Yeah, it isn't released to npm yet -- it will be released on the next release. @anikethsaha @Xtremilicious could you make a PR that updates the theme list table on the documentation to add this theme? |
Yes, that link, and this link: https://react-jsonschema-form.readthedocs.io/en/latest/usage/themes/#supported-themes |
feat: Add Bootstrap 4 Theme support (rjsf-team#1938)
@rjsf/bootstrap-4 has just been published to npm with version 2.3.0. Thanks for your patience! |
Reasons for making this change
To add Bootstrap 4 theme support for react-jsonschema-form as a task for the MLH Fellowship 2020.
Fix #1455
Checklist