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

Use react-select in variable dropdown for better UX #4675

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

robinmolen
Copy link
Contributor

@robinmolen robinmolen commented Sep 17, 2024

Closes #4524

Changes

The variable dropdown now uses the react-select component. Allowing for searching within the options

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

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 62.16216% with 14 lines in your changes missing coverage. Please review.

Project coverage is 96.56%. Comparing base (ff04630) to head (827f6ac).
Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
src/openforms/formio/migration_converters.py 17.64% 13 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4675      +/-   ##
==========================================
- Coverage   96.61%   96.56%   -0.05%     
==========================================
  Files         746      746              
  Lines       25144    25174      +30     
  Branches     3302     3310       +8     
==========================================
+ Hits        24292    24309      +17     
- Misses        589      601      +12     
- Partials      263      264       +1     

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

@robinmolen
Copy link
Contributor Author

The chromatic tests are failing because either the DMN variable options aren't loaded yet, or for some reason the DMN select cannot be done correctly (This second fail is also inconsistent. Sometimes it succeeds, sometimes it fails...)

Maybe we should remove the dmn variable select check from the "Empty" DMN Action configuration test scenario

@sergei-maertens sergei-maertens changed the title Feature/4524 react select in variable dropdown Use react-select in variable dropdown for better UX Sep 19, 2024
@sergei-maertens sergei-maertens force-pushed the feature/4524-react-select-in-variable-dropdown branch from 714b630 to 7519e29 Compare September 19, 2024 15:11
}

// The selected option only shows the label
.admin-react-select__value-container & {
Copy link
Member

@sergei-maertens sergei-maertens Sep 24, 2024

Choose a reason for hiding this comment

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

we have src/openforms/scss/vendor/_react-select.scss, but I need to take a proper look at how this sass is structured still! more feedback will come

edit: nvm, I noticed you found that file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I put it here to keep all the .form-variable-dropdown styling contained to this file :)

Comment on lines 29 to 31
.admin-react-select__menu {
z-index: 10;
}
Copy link
Member

Choose a reason for hiding this comment

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

magic z-index numbers usually hint at the stacking context not being as expected - perhaps there's a better solution?

@stevenbal this may also be what you are running into with the prefill modal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look if this can be solved in another way 👍

Copy link
Contributor Author

@robinmolen robinmolen Sep 25, 2024

Choose a reason for hiding this comment

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

This was needed, because the .submit-row would be on-top of the react-select menu. The z-index on the .submit-row was added because of an issue with the z-index of the tinymce.

I've changed the styling of the .submit-row and the tinymce to prevent this stacking issue. (And removed this z-index styling from the .admin-react-select__menu)

@robinmolen robinmolen force-pushed the feature/4524-react-select-in-variable-dropdown branch from daa0d1a to b84481b Compare September 25, 2024 10:14
@sergei-maertens
Copy link
Member

Closing in favour of #4730

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 a React dropdown out of the choose form variable-dropdown
2 participants