-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(drag-utils): add utility function for cloning array items from one array to another #13743
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
Conversation
…ne array to another creates a utility helper for cloning an array item into the array of the drop container compliments moveItemInArray and transferArrayItem partially closes angular#13100
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 package-lock
should be removed since we're using Yarn.
src/cdk/drag-drop/drag-utils.ts
Outdated
* @param currentIndex Index of the item in its current array. | ||
* @param targetIndex Index at which to insert the item. | ||
* | ||
* PR NOTE: I followed the pattern of the other two functions here. It would be simpler and more |
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.
Remove this note.
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.
Done, what do you think about the change in implementation the note suggested, passing the array element (currentElement) instead of passing currentIndex and currentArray?
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.
I made all recommended changes. The only open item is whether to change implementation to pass currentElement
instead of currentIndex
and currentArray
to the function. I'll resubmit and change if requested.
…ne array to another resolves issues surfaced from code review
…ne array to another cuts extra space between parens
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.
LGTM
…ne array to another (#13743) * feat(drag-utils): add utility function for cloning array items from one array to another creates a utility helper for cloning an array item into the array of the drop container compliments moveItemInArray and transferArrayItem partially closes #13100 * feat(drag-utils): add utility function for cloning array items from one array to another resolves issues surfaced from code review * feat(drag-utils): add utility function for cloning array items from one array to another cuts extra space between parens * removes link in comments, and clarifies function definition as recommended in review * resolves nits
…ne array to another (angular#13743) * feat(drag-utils): add utility function for cloning array items from one array to another creates a utility helper for cloning an array item into the array of the drop container compliments moveItemInArray and transferArrayItem partially closes angular#13100 * feat(drag-utils): add utility function for cloning array items from one array to another resolves issues surfaced from code review * feat(drag-utils): add utility function for cloning array items from one array to another cuts extra space between parens * removes link in comments, and clarifies function definition as recommended in review * resolves nits
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
creates a utility helper for cloning an array item into the array of the drop container
compliments moveItemInArray and transferArrayItem
partially closes #13100 a fix for the UI aspect of this issue will be submitted in the next day or two