Skip to content

feat(drag-utils): add utility function for cloning array items from one array to another - #13743

Merged
mmalerba merged 5 commits into
angular:masterfrom
nayfin:clone-item-to-list
Oct 25, 2018
Merged

feat(drag-utils): add utility function for cloning array items from one array to another#13743
mmalerba merged 5 commits into
angular:masterfrom
nayfin:clone-item-to-list

Conversation

@nayfin

@nayfinnayfin commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

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

@nayfin
feat(drag-utils): add utility function for cloning array items from o…
6797fbb
…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 closesangular#13100
@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 22, 2018
crisbeto
crisbeto reviewed Oct 22, 2018
View reviewed changes

@crisbetocrisbeto left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
Comment threadsrc/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

@crisbetocrisbetoOct 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this note.

@nayfinnayfinOct 23, 2018

Copy link
Copy Markdown
ContributorAuthor

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?

Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
nayfin
nayfin commented Oct 23, 2018
View reviewed changes

@nayfinnayfin left a comment

Copy link
Copy Markdown
ContributorAuthor

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.

Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
@nayfin
feat(drag-utils): add utility function for cloning array items from o…
1e30514
…ne array to another
resolves issues surfaced from code review
@nayfin
feat(drag-utils): add utility function for cloning array items from o…
436cffb
…ne array to another
cuts extra space between parens
crisbeto
crisbeto reviewed Oct 23, 2018
View reviewed changes
Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
crisbeto
crisbeto approved these changes Oct 24, 2018
View reviewed changes

@crisbetocrisbeto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment threadsrc/cdk/drag-drop/drag-utils.ts Outdated
@crisbetocrisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Oct 24, 2018
@nayfinnayfin mentioned this pull request Oct 24, 2018
Closed
@mmalerba
mmalerba merged commit d2d8a1f into angular:masterOct 25, 2018
mmalerba pushed a commit that referenced this pull request Oct 26, 2018
@nayfin@mmalerba
feat(drag-utils): add utility function for cloning array items from o…
13395c5
…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
@nayfin
nayfin deleted the clone-item-to-list branch October 28, 2018 01:34
atscott pushed a commit to atscott/components that referenced this pull request Nov 5, 2018
@nayfin@atscott
feat(drag-utils): add utility function for cloning array items from o…
0f50dda
…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 closesangular#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
@angular-automatic-lock-bot

angular-automatic-lock-botBot commented Sep 10, 2019

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 10, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbetocrisbetocrisbeto approved these changes
@jelbournjelbournAwaiting requested review from jelbourn

Assignees

No one assigned

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreementtarget: patchThis PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cdk-drop: allow drag-and-drop copy behavior

4 participants

@nayfin@crisbeto@googlebot@mmalerba