-
Notifications
You must be signed in to change notification settings - Fork 26
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
👽 [#4693] Integrate Objects API prefill modal with backend #4799
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4799 +/- ##
==========================================
+ Coverage 96.57% 96.59% +0.02%
==========================================
Files 747 747
Lines 25400 25400
Branches 3355 3355
==========================================
+ Hits 24529 24535 +6
+ Misses 608 604 -4
+ Partials 263 261 -2 ☔ View full report in Codecov by Sentry. |
c725d4c
to
7436b8f
Compare
@sergei-maertens I'm trying to fix the issue with the react-select menu being clipped, but I'm not sure how to fix it. I've tried setting a z-index, but that alone doesn't fix the issue. If I try using current situation Any ideas on how this can be fixed? Changing the menuPlacement to |
49af4a9
to
175af2c
Compare
@sergei-maertens I've made the placement bottom and set a maxMenuHeight for the selects used in the modal to avoid overflow issues (efb307d) |
src/openforms/js/components/admin/form_design/variables/prefill/ObjectsAPIFields.js
Outdated
Show resolved
Hide resolved
src/openforms/js/components/admin/form_design/variables/prefill/ObjectsAPIFields.js
Show resolved
Hide resolved
src/openforms/js/components/admin/form_design/variables/prefill/ObjectsAPIFields.js
Outdated
Show resolved
Hide resolved
src/openforms/js/components/admin/form_design/variables/prefill/ObjectsAPIFields.js
Outdated
Show resolved
Hide resolved
src/openforms/js/components/admin/form_design/variables/prefill/ObjectsAPIFields.js
Outdated
Show resolved
Hide resolved
src/openforms/js/components/admin/forms/objects_api/ObjectTypeSelect.js
Outdated
Show resolved
Hide resolved
0b8256e
to
5e646cc
Compare
5e646cc
to
33836d3
Compare
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've noticed a couple more things when checking this out locally to look into the dropdown issue:
- I'm missing the code structure from the registrations app a bit - now there's a single file with a number of components for the Objects API form, all in the
variables/prefill
folder. Perhaps building out a sub-structure like inregistrations
with a directory per plugin ID would be better to manage the code, and we can then split out the individual components. - When resetting the api group in the prefill form, the selected object type and version are not reset, only the variables mapping. This leads to hidden form state that is not accurately represented in the UI and is confusing at the least and dangerous at most.
- The form field/button to copy the settings from a registration backend I haven't tested yet, but I found the UI controls to be in an odd place. I would expect that to be the first thing I see, visually separated from the rest of the fields since they also affect API group, object type and object type version, not just the variables mapping. And a paragraph with some explanation of what it actually does would go a long way for users to decide what they want to do to configure. Additionally, the 'copy' language should maybe be more along the lines of 'synchronize', since this would also be the mechanism to update the prefill config after the registration variables config/mapping is updated without having to manage everything manually afterwards
- The help texts for fields now have the registration field help texts, those either need to be removed for the prefill plugin, or overridden with prefill-specific content. Now it's just confusing instead of making things more clear.
@@ -82,6 +82,7 @@ const SelectWithoutFormik = ({name, options, value, className, onChange, ...prop | |||
classNamePrefix="admin-react-select" | |||
styles={styles} | |||
menuPlacement="auto" | |||
menuPortalTarget={document.body} |
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 is too simple as it breaks storybook, which renders in another DOM node and the within(canvasElement)
queries break. Instead, we should probably set up a custom context that defaults to document.body
, but which we can override in storybook to point it to the right node, similar to what we do with the modals.
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 now have this modal context for storybook from @robinmolen, so we can apply the same pattern.
that allows the user to copy the configuration used by the Form's registration backend
* remove unnecessary prefill prefix from options in javascript code * remove trailing comments * use variable for computed name in VariableMapping * add dropdown to select a registration backend instead of always picking first * also copy variablesmapping when clicking copy button * use setValues to set formik values at once, instead of multiple calls of setFieldValue
8e23cf3
to
4716f4b
Compare
Rebased on master - I will now look at the state again and do a thorough review! |
I'm not happy yet with the "copy configuration" button and this is a tough nut to crack. After some discussion/debate with Joeri, we've settled on something like this:
Please also set up the styling properly so that the link and button are nicely aligned. |
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 should go through these remarks tomorrow :)
Let's get this PR wrapped up before we tackle the other one.
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 we structure/organize this code a bit better? I noticed I was getting lost on what calls what. I'd like to see:
- a directory
src/openforms/js/components/admin/form_design/variables/prefill/
with... - subdirectories
default
,objects_api
AttributeField
,IdentifierRoleField
andDefaultFields
go indefault
ObjectsAPIFields
goes inobjects_api
, and you can break up the component in multiple files - try to go for one file per component- Consider moving
PLUGIN_COMPONENT_MAPPING
to aconstants.js
orindex.js
instead of defining it inPrefillConfigurationForm
(in theprefill
directory)
This mimics the structure of registration
frontend components more closely and makes it a bit easier to navigate imo.
className="button" | ||
onClick={e => { | ||
e.preventDefault(); | ||
const confirmed = window.confirm( |
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.
the useConfirm
hook rework was merged, so you can use that now :)
@@ -104,7 +190,7 @@ const ObjectsAPIFields = ({errors}) => { | |||
<ObjectsAPIGroup | |||
apiGroupChoices={apiGroups} | |||
onChangeCheck={async () => { | |||
if (values.options.variablesMapping.length === 0) return true; | |||
if (variablesMapping.length === 0) return true; |
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 change resets the objecttype and object version too, so just checking for variable mappings is maybe too naive? Maybe better to warn as soon as an objecttype is selected
@@ -123,11 +211,11 @@ const ObjectsAPIFields = ({errors}) => { | |||
} | |||
> | |||
<ObjectTypeSelect |
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.
the prefill configuration now shows the tooltips/help texts for the registration configuration, which is misleading
@@ -147,9 +237,11 @@ const ObjectsAPIFields = ({errors}) => { | |||
<ObjectTypeVersionSelect |
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.
the prefill configuration now shows the tooltips/help texts for the registration configuration, which is misleading
const defaults = { | ||
objectsApiGroup: '', | ||
objecttype: '', | ||
objecttypeUuid: '', | ||
objecttypeVersion: null, | ||
variablesMapping: [], | ||
}, | ||
errors, | ||
}) => { | ||
}; | ||
options = {...defaults, ...options}; |
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 think this needs to be pushed down to ObjectsAPIFields
and have it set the default values on initial render - you can get the initialValues
from the Formik state. This generic component should not be polluted with plugin-specific options and we want to keep the relevant code co-located.
@@ -82,6 +82,7 @@ const SelectWithoutFormik = ({name, options, value, className, onChange, ...prop | |||
classNamePrefix="admin-react-select" | |||
styles={styles} | |||
menuPlacement="auto" | |||
menuPortalTarget={document.body} |
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 now have this modal context for storybook from @robinmolen, so we can apply the same pattern.
Closes #4693
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh
./bin/compilemessages_js.sh
Commit hygiene