-
Notifications
You must be signed in to change notification settings - Fork 130
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
[WIP]fix(platform): cache user selection after searching for new user list for approval flow #12505
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fundamental-ngx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Visit the preview URL for this PR (updated for commit 8efde6d): https://fundamental-ngx-gh--pr12505-fix-approval-flow-se-7r9odk1q.web.app (expires Wed, 09 Oct 2024 09:11:50 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff |
this.selectedItems = this._selectionModel.selected; | ||
const event = new SelectionChangeEvent(); | ||
event.selectedItems = this.selectedItems; | ||
if (selection.added?.length) { | ||
event.added = selection.added[0]; |
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.
So, this change does fix the issue we are seeing for the approval flow component. However, I think this code could pose a problem in the future for the list component, agnostic to the approval flow. It is interesting that added
on a selectionChangeEvent is always just one BaseListItem
but selectionModel emits an array of them. Which makes me think, instead of doing event.added = selection.added[0];
we need to either:
- Iterate over
selection.added
and emit aselectedItemChange
event for eachselection.item
- Or, update the
selectedItemChange
event to be able to pass either a single item (as it is now), or emit an array of them
Same idea goes for selection.removed
as well. I'm inclined to say we should go for the first bullet point, as to not have to update the selectedItemChange
api.
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.
comment above
closes #12093
ng-15 #12107