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

👽 [#4693] Integrate Objects API prefill modal with backend #4799

Merged
merged 14 commits into from
Nov 22, 2024

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Oct 28, 2024

Closes #4693

Changes

  • Integrate Objects API prefill modal with backend

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

@stevenbal stevenbal marked this pull request as draft October 28, 2024 14:49
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (cf95065) to head (5e269c9).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4799      +/-   ##
==========================================
+ Coverage   96.58%   96.60%   +0.02%     
==========================================
  Files         749      749              
  Lines       25540    25540              
  Branches     3377     3377              
==========================================
+ Hits        24668    24674       +6     
+ Misses        608      604       -4     
+ Partials      264      262       -2     

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


🚨 Try these New Features:

@stevenbal stevenbal force-pushed the feature/4693-product-prefill-modal-integration branch from c725d4c to 7436b8f Compare October 28, 2024 15:37
@stevenbal
Copy link
Contributor Author

@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 overflow: visible, I do see the full menu, but the rest of the modal breaks :/

current situation
image
with overflow
image

Any ideas on how this can be fixed? Changing the menuPlacement to bottom will likely cause the same issue in the future, if the list of choices gets long enough

@stevenbal stevenbal force-pushed the feature/4693-product-prefill-modal-integration branch 6 times, most recently from 49af4a9 to 175af2c Compare October 29, 2024 10:59
@stevenbal stevenbal marked this pull request as ready for review October 29, 2024 11:22
@stevenbal
Copy link
Contributor Author

@sergei-maertens I've made the placement bottom and set a maxMenuHeight for the selects used in the modal to avoid overflow issues (efb307d)

@stevenbal stevenbal marked this pull request as draft October 29, 2024 15:33
@stevenbal stevenbal force-pushed the feature/4693-product-prefill-modal-integration branch 3 times, most recently from 0b8256e to 5e646cc Compare November 4, 2024 08:35
@stevenbal stevenbal marked this pull request as ready for review November 4, 2024 08:43
@sergei-maertens sergei-maertens self-assigned this Nov 8, 2024
@sergei-maertens sergei-maertens force-pushed the feature/4693-product-prefill-modal-integration branch from 5e646cc to 33836d3 Compare November 12, 2024 10:15
Copy link
Member

@sergei-maertens sergei-maertens left a 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 in registrations 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.

@sergei-maertens sergei-maertens marked this pull request as draft November 12, 2024 11:48
@sergei-maertens sergei-maertens force-pushed the feature/4693-product-prefill-modal-integration branch from 8e23cf3 to 4716f4b Compare November 18, 2024 13:32
@sergei-maertens
Copy link
Member

Rebased on master - I will now look at the state again and do a thorough review!

@sergei-maertens
Copy link
Member

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:

  1. When you open the prefill modal, you get the current state (with dropdowns likely empty if this is the initial configuration:

    image

    • A link is injected next to the 'plugin' field. This will require some interesting changes which we can discuss tomorrow, since you currently can't provide additional children for the plugin field... We can add a tooltip here that describes how it works/what it does for better documentation.
    • The registration backend dropdown is not visible at all
  2. The user clicks the link to copy the settings - this makes the registration dropdown visible:

    image

  3. Clicking the link again just collapse the form row again

  4. Selecting a backend and then clicking the 'copy' button performs the copy as it is right now, but also automatically collapses the form row again at the end to declutter the UI again.

Please also set up the styling properly so that the link and button are nicely aligned.

Copy link
Member

@sergei-maertens sergei-maertens left a 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.

@stevenbal stevenbal force-pushed the feature/4693-product-prefill-modal-integration branch from a9da278 to 97b7f23 Compare November 21, 2024 11:17
@sergei-maertens sergei-maertens marked this pull request as ready for review November 22, 2024 11:02
stevenbal and others added 12 commits November 22, 2024 12:04
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
* use new confirmation modal for prefill config copy button warning
* add decorator for reactselect to fix storybook tests
* move default prefill fields and objects api fields to separate directories
* allow passing help texts/labels to reused Objects API fields (since they are no longer registration specific)
* fix confirmation check for objects API prefill fields (previously the value was changed before clicking accept)
* move initialization of Objects API prefill options to ObjectsAPIFields component (was previously in generic component)
and automatically hide the select after copying
because the options menu is now portaled and appended to document.body, the query in the tests should change as well
@sergei-maertens sergei-maertens force-pushed the feature/4693-product-prefill-modal-integration branch from 1e6ab24 to 7a0cdc9 Compare November 22, 2024 11:04
This was a leftover in case window.confirm is/was used - this has all
been replaced with the custom confirmation hook and modal which must
always be async.

Discussed via Slack.
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Let's merge this so we can get it tested in the alpha release!

@sergei-maertens sergei-maertens force-pushed the feature/4693-product-prefill-modal-integration branch from 4dfcbda to 86eb49b Compare November 22, 2024 13:16
The tests broke because of react-select now *always* being rendered
in a portal. Added a helper that encapsulates this logic so that
we don't need to worry about this in our actual test code.
@sergei-maertens sergei-maertens force-pushed the feature/4693-product-prefill-modal-integration branch from 86eb49b to 5e269c9 Compare November 22, 2024 13:32
@sergei-maertens sergei-maertens merged commit d985471 into master Nov 22, 2024
34 checks passed
@sergei-maertens sergei-maertens deleted the feature/4693-product-prefill-modal-integration branch November 22, 2024 14:02
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.

Product prefill configuration modal: integrate with backend
3 participants