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

bug(refs T32855): Fix multiselect #1393

Merged
merged 8 commits into from
May 24, 2023
Merged

bug(refs T32855): Fix multiselect #1393

merged 8 commits into from
May 24, 2023

Conversation

gruenbergerdemos
Copy link
Contributor

@gruenbergerdemos gruenbergerdemos commented May 23, 2023

Ticket: https://yaits.demos-deutschland.de/T32855

DpMultiselect was changed from a functional component to a template-component, because functional components won't work in vue3.
We also need to adjust the slot props here, because we now have an additional child that passes on the props.

How to review/test

Check some multiselects (eg. Verfahren -> Grundeinstellungen -> Datenerfassende Organisation).

Linked PRs (optional)

demos-europe/demosplan-ui#237

@gruenbergerdemos gruenbergerdemos self-assigned this May 23, 2023
@gruenbergerdemos gruenbergerdemos changed the base branch from main to release May 23, 2023 13:14
@gruenbergerdemos gruenbergerdemos added type:bug Something isn't working review:frontend PRs that are missing review(s) from a FE perspective labels May 23, 2023
@gruenbergerdemos gruenbergerdemos marked this pull request as ready for review May 23, 2023 13:43
:value="selectedGroups"
@input="selectGroups">
<template v-slot:option="{ option }">
<span>{{ option.title }}</span>
<template v-slot:option="{ props }">
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to pass in the whole props object here? See question in other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because otherwise we can't access the remove function. Or do you have an idea about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, not at the first glance.

:value="currentUserOrga"
:searchable="true"
:id="userId + ':organisationId'"
:close-on-select="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is closeOnSelect used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in the vue multiselect. I added a prop in DpMultiselect (default true). 24fc29a

client/js/components/user/DpUserFormFields.vue Outdated Show resolved Hide resolved
gruenbergerdemos and others added 4 commits May 24, 2023 12:52
…al.vue

Co-authored-by: hwiem <40487668+hwiem@users.noreply.github.com>
…r.vue

Co-authored-by: hwiem <40487668+hwiem@users.noreply.github.com>
Default for searchable is true.
Default for closeOnSelect is true.
@gruenbergerdemos gruenbergerdemos added the lifecycle:do-not-merge Pull request is just an experiment and not meant to be merged label May 24, 2023
Copy link
Contributor

@spiess-demos spiess-demos left a comment

Choose a reason for hiding this comment

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

lgtm (superficial code review).

Copy link
Contributor

@spiess-demos spiess-demos left a comment

Choose a reason for hiding this comment

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

lgtm (superficial code review).

Copy link
Contributor

@hwiem hwiem left a comment

Choose a reason for hiding this comment

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

lgtm, code review only

@graupnerdemos graupnerdemos merged commit cbfc96f into release May 24, 2023
@graupnerdemos graupnerdemos deleted the b_T32855_dropdown branch May 24, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle:do-not-merge Pull request is just an experiment and not meant to be merged review:frontend PRs that are missing review(s) from a FE perspective type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants