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

Convert ZGW registration options to modal + Formik #4607

Merged

Conversation

sergei-maertens
Copy link
Member

@sergei-maertens sergei-maertens commented Aug 17, 2024

Partially closes #4606 - does the groundwork before we can actually add the real UX improvements with nice dropdowns.

Changes

  • Refactored form fields to Formik
  • Moved fields/config form into modal
  • Moved case property mappings from modal to a tab inside the new modal
  • Fixed some behaviours in the Objects API that were uncovered through testing ZGW
  • Updated some error-state styling for react-select

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@sergei-maertens sergei-maertens force-pushed the feature/4606-improve-zgw-registration-options-ux branch from 3fb2627 to cc9e911 Compare August 17, 2024 21:55
Copy link

codecov bot commented Aug 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.65%. Comparing base (515a8a7) to head (b1b484c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4607   +/-   ##
=======================================
  Coverage   96.65%   96.65%           
=======================================
  Files         727      727           
  Lines       24406    24406           
  Branches     3213     3213           
=======================================
  Hits        23589    23589           
  Misses        564      564           
  Partials      253      253           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergei-maertens sergei-maertens force-pushed the feature/4606-improve-zgw-registration-options-ux branch 4 times, most recently from 837b52a to de02d7e Compare August 19, 2024 07:40
@@ -132,7 +132,7 @@ JSONTemplateField.propTypes = {
helpText: PropTypes.node,
};

const ContentJSON = () => (
export const ContentJSON = () => (
Copy link
Member Author

Choose a reason for hiding this comment

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

re-using this here as-is because it's legacy - I'm hoping we can remove all of this in 3.0 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be in another file since it's used in ZGW as well?Or this will be done when we remove it in v3.0?

Copy link
Member Author

@sergei-maertens sergei-maertens Aug 19, 2024

Choose a reason for hiding this comment

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

I think for 3.0 we need to either remove the objects API integration in ZGW APIs (this was a temporary solution for Utrecht if I remember correctly), or convert it to the Objects API variable mapping approach. Either way, the contentJson template is gone in 3.0. That's why I didn't find it worth it to move this to a shared module instead, considering it should only exist here for another 3 months and then it's gone.

So yeah, it should, but in this case I don't think it's worth the effort

@sergei-maertens sergei-maertens requested a review from vaszig August 19, 2024 07:52
@sergei-maertens sergei-maertens force-pushed the feature/4606-improve-zgw-registration-options-ux branch from de02d7e to b1b484c Compare August 19, 2024 07:55
@sergei-maertens sergei-maertens marked this pull request as ready for review August 19, 2024 07:55
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

And a comment concerning frontend..not so important imo but if it's an easy fix it would be nice (It happens both in storybook and in the admin)

image

@@ -132,7 +132,7 @@ JSONTemplateField.propTypes = {
helpText: PropTypes.node,
};

const ContentJSON = () => (
export const ContentJSON = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be in another file since it's used in ZGW as well?Or this will be done when we remove it in v3.0?

/>

<SubmitRow>
<SubmitAction
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this since the SubmitRow contains SubmitAction and event.preventDefault()?It's a bit confusing to me..

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to make sure that Formik's handleSubmit gets called, since it will run (client-side) validation en then perform the onSubmit if everything is fine. So we don't let the browser default behaviour submit to the backend, as we need to handle everything in JS (that's what the event.preventDefault() is for).

{({handleSubmit}) => (
<>
<ZGWFormFields
index={index}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see index being used somewhere anymore

@sergei-maertens
Copy link
Member Author

And a comment concerning frontend..not so important imo but if it's an easy fix it would be nice (It happens both in storybook and in the admin)

image

yeah, this is a known issue - I've looked into fixing it but overlaying the icon on the tab is also ugly. I think in a future iteration we'll just have to include the icon inline with the text, fully inside the tab and that will be the best solution. Out of scope for now!

There are some slight tweaks in literals here and there, and the
selects are now react-selects that can be searched, but other than that
they're pretty much equivalent to what we had before.
react-select sets the value to null, which doesn't have a property
'value', leading to crashes. Instead, we set the formik field value
to 'undefined', which effectively clears it in the formik state.
* The modal spacing is a cheeky workaround for tab error icons that would
  otherwise be cut off by the overflow: auto due to the desired scroll
  behaviour inside a form modal.
* The react-select styles ensure the border color of the select is consistent
  with other input types in the django admin style (red border if there are
  validation errors)
* The changelist styles ensure that items in a table row with validation
  errors don't break the (vertical) alignment between them - as soon as
  any has a validation error, its content will be pushed down so we need
  to align to the bottom of the table cell (like in the variables table
  styles).
The layout is still the same, except this table will no longer live in its own
modal, but be a tab in the main modal for the registration options instead.

The fields now make use of Formik's FieldArray to manage a list of nested
objects.
Moving the ZGW options into a modal allows us to make use of Formik,
since the fields themselves are now properly isolated and we can
add an explicit 'submit' button to commit the configuration back
to the main state. This applies a similar set up like in the objects
API registration options.

The configuration is now broken into parts too - base ZGW options
go in the first tab, while the case property configuration is
moved to its own tab rather than sticking it in a modal itself (
modal in modal is not a user friendly UI).

I'm not sure if putting the objects API options in a fieldset or
maybe its own tab is the best approach, we can discuss.
* Added case with validation errors
* Added some more options to confidentiality level select
* setValues takes a callback for previous values, so that we can avoid
  form data races/stale values
* removed unused import
* fixed the handling for setting the default API group, the ternary
  was not being evaluated as expected (uncovered via ZGW options and
  this applies the fix here too)
@sergei-maertens sergei-maertens force-pushed the feature/4606-improve-zgw-registration-options-ux branch from b1b484c to df4ecee Compare August 19, 2024 15:55
@sergei-maertens sergei-maertens merged commit d46319d into master Aug 19, 2024
13 of 16 checks passed
@sergei-maertens sergei-maertens deleted the feature/4606-improve-zgw-registration-options-ux branch August 19, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ZGW registration options consistent with Objects API
2 participants