-
Notifications
You must be signed in to change notification settings - Fork 21
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
Inactive records in entity select #2056
Conversation
Deployed to https://pr-2056.aam-digital.net/ |
Unfortunately making the message "None, but XX inactive found" itself clickable hasn't worked out. But UI-wise it should also be OK if this message is only a non-clickable hint and if the user has to click on the option with the checkbox in the entry below. |
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 functionality already looks good. But I would have a little wish-list to make the component a little clearer
- I don't see a reason why the
filter
function every time applies theadditionalFilter
and theisSelected
check. This rarely changes. Can we instead create arelevantEntities
property that only holds the entities which pass these two checks, so thefilter
function only does the filtering based on the search term. - I don't like the multiple calls of the
getNumberOfInactive
function and also the complicated way to get this number. Let's rather have two arraysfilteredEntities
which holds the actually displayed entities (as it is currently) andinactiveFilteredEntities
which only holds the inactive ones that match the filter. Both are calculated based on therelevantEntities
, as described before, inside thefilter
function. This way we drastically remove the amount of times this has to be calculated and it also makes the code a little more straight forward.
Let me know if you need more details.
src/app/core/common-components/entity-select/entity-select.component.html
Outdated
Show resolved
Hide resolved
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.
@christophscheuing thanks for implementing this! Looks pretty good to me now. I had a few questions and small remarks still, however.
src/app/core/common-components/entity-select/entity-select.component.spec.ts
Outdated
Show resolved
Hide resolved
src/app/core/common-components/entity-select/entity-select.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/common-components/entity-select/entity-select.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/common-components/entity-select/entity-select.component.html
Outdated
Show resolved
Hide resolved
# Conflicts: # src/app/core/common-components/entity-select/entity-select.component.spec.ts # src/app/core/common-components/entity-select/entity-select.component.ts
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.
Great 👍
🎉 This PR is included in version 3.31.1-master.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.31.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closes #1983
Functionality already there, but still working on the tests.