-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add LayoutGridField and LayoutMultiSchemaField #4506
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
Added new `LayoutGridField` and `LayoutMultiSchemaField` with updates to `utils` to support them - Updated `@rjsf/utils` to support the new fields as follows: - Updated the `UIOptionsBaseType` to add the optional `optionsSchemaSelector?: string` prop - Updated `getDiscriminatorFieldFromSchema()` to use the `DISCRIMINATOR_PATH` constant - Updated `hashForSchema.ts` to expose the `sortedJSONStringify()` method adding it to the `index.ts` export - Also added unit tests for the exposed method - Updated `getTestIds()` to read the `NODE_ENV` from the `process.env` via lodash `get()` to avoid odd bug - Updated `optionsList()` to switch the order of the generics and to add support for drilling into a `anyOf/oneOf` schema with a discriminator or a `optionsSchemaSelector` - This resulted in a few updates to other methods - Updated the tests to match the new capabilities - Updated `findSelectedOptionInXxxOf()` to use `getDiscriminatorFieldFromSchema()` rather than `DISCRIMINATOR_PATH` - Updated `ensureFormDataMatchingSchema()` to add the missing generics on the functions that have them - In `rjsf/core` : - Updated `package.json` to add the necessary `@testing-library` packages needed for the new components tests - Updated `ArrayField`, `BooleanField` and `StringField` to fix the order of the generics for `optionsList()` - Updated `RadioWidget` to add `radiogroup` role and `SelectWidget` to add the `combobox` role - Added the `LayoutGridField` and `LayoutMultiSchemaField` to the `fields` and added full `@testing-library` tests for them - Updated the snapshots due to the `role` changes to the two widgets - Updated the `utility-functions.md` to add `sortedJSONStringify()` docs and updated the `optionsList()` docs - Updated the `v6.x upgrade guide.md` to add breaking changes for `optionsList()` and adding the new `sortedJSONStringify()`
import get from 'lodash/get'; | ||
import has from 'lodash/has'; | ||
import isEmpty from 'lodash/isEmpty'; | ||
import omit from 'lodash/omit'; | ||
import set from 'lodash/set'; |
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 there a specific reason these imports are direct imports and not package imports? There are a couple of other places in the repo that use direct imports form lodash
too. You can global search lodash/
import get from 'lodash/get'; | |
import has from 'lodash/has'; | |
import isEmpty from 'lodash/isEmpty'; | |
import omit from 'lodash/omit'; | |
import set from 'lodash/set'; | |
import { get, has, isEmpty, omit, set } from 'lodash/get'; |
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.
RJSF uses imports this way because not everyone has tree-shaking set up properly and it helps reduce the size of the distribution
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.
That makes sense, then should all import from lodash
be direct imports to make it consistent? I see some imports from lodash that are package imports.
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.
It's not important in the tests, just the code that is packaged up and published
const widgetOptions = { enumOptions, ...uiOptions }; | ||
|
||
return ( | ||
<> |
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.
Why do we need fragment here? Can we just render Widget
?
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.
Because it also, optionally renders the FieldErrorTemplate
as a sibling to the Widget
); | ||
} | ||
|
||
// eslint-disable-next-line no-unused-vars |
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.
This can be removed since LOOKUP_MAP
is being used in REGISTRY_FORM_CONTEXT
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.
It's for the props
variable on the type definition.
import validator from '@rjsf/validator-ajv8'; | ||
import { render, screen, within } from '@testing-library/react'; | ||
import userEvent from '@testing-library/user-event'; | ||
import { get, has, omit, pick } from 'lodash'; |
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.
Change these to direct imports?
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.
Nah, they are in tests
} from '@rjsf/utils'; | ||
import { render, screen, waitFor, within } from '@testing-library/react'; | ||
import userEvent from '@testing-library/user-event'; | ||
import { get } from 'lodash'; |
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.
change to direct import?
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.
Nah, they are in tests
Reasons for making this change
Added new
LayoutGridField
andLayoutMultiSchemaField
with updates toutils
to support them@rjsf/utils
to support the new fields as follows:UIOptionsBaseType
to add the optionaloptionsSchemaSelector?: string
propgetDiscriminatorFieldFromSchema()
to use theDISCRIMINATOR_PATH
constanthashForSchema.ts
to expose thesortedJSONStringify()
method adding it to theindex.ts
exportgetTestIds()
to read theNODE_ENV
from theprocess.env
via lodashget()
to avoid odd bugoptionsList()
to switch the order of the generics and to add support for drilling into aanyOf/oneOf
schema with a discriminator or aoptionsSchemaSelector
findSelectedOptionInXxxOf()
to usegetDiscriminatorFieldFromSchema()
rather thanDISCRIMINATOR_PATH
ensureFormDataMatchingSchema()
to add the missing generics on the functions that have themrjsf/core
:package.json
to add the necessary@testing-library
packages needed for the new components testsArrayField
,BooleanField
andStringField
to fix the order of the generics foroptionsList()
RadioWidget
to addradiogroup
role andSelectWidget
to add thecombobox
roleLayoutGridField
andLayoutMultiSchemaField
to thefields
and added full@testing-library
tests for themrole
changes to the two widgetsutility-functions.md
to addsortedJSONStringify()
docs and updated theoptionsList()
docsv6.x upgrade guide.md
to add breaking changes foroptionsList()
and adding the newsortedJSONStringify()
tsconfig.base.json
to addskipLibCheck: true
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.