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

Drag and Drop functionality is added #1504

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mehdimohammadijan
Copy link

@mehdimohammadijan mehdimohammadijan commented Dec 8, 2021

I have added Drag and Drop functioanlity by incorporating Vue Draggable library (https://github.com/SortableJS/Vue.Draggable).

screen cast: https://watch.screencastify.com/v/sxxf2JcWVYR52ZvG8EHO

Drag1

Drag2

@raystorm-place
Copy link

Everyone waited long for the drag and drop feature! 🚀

@slaweet
Copy link

slaweet commented Jan 13, 2022

@mehdimohammadijan It's great that you worked on adding the drag'n'drop functionality! 🥇

I was trying to use your branch and I have the following feedback:

  • It contains a lot of unrelated formatting changes (e.g. spaces, new lines, quotes) - this makes it super hard for whoever is going to review this to see what the actual relevant changes are.
  • When drag'n'dropping an item, it doesn't emit the 'input' event for me, so it doesn't update the array value I'm passing to vue-multiselect. I was trying to read the changed code to understand why, but didn't get very far because of the unrelated formatting changes mentioned above.
  • I think there should be a boolean flag to control if drag'n'drop is enabled (probably with default: false) as I don't want all selects to be re-orderable.

@slaweet
Copy link

slaweet commented Jan 13, 2022

@mehdimohammadijan I would appreciate it if you also linked/published the code you used to make the screencast. Seeing what are the differences from how I use vue-multiselect would help me to figure out why it doesn't work (update the array value) for me.

selectedOptions: {
deep: true,
handler: function (newVal) {
this.$emit("addtag", newVal);
Copy link

@slaweet slaweet Jan 13, 2022

Choose a reason for hiding this comment

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

I don't understand why the emitted event should be addtag, not the already used elsewhere input event

Suggested change
this.$emit("addtag", newVal);
this.$emit("input", newVal);

Comment on lines 360 to 371
// the watch will look to any changes in value to adjust selectedOptions for adding and removing values
immediate: true,
handler(newVal, oldVal) {
if (newVal && oldVal) {
if (newVal.length > oldVal.length) {
this.selectedOptions.push(newVal[newVal.length - 1]);
} else {
this.selectedOptions = this.selectedOptions.filter((option) =>
newVal.includes(option)
);
}
}
Copy link

Choose a reason for hiding this comment

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

value is coming from outside, so it can also be changed to a completely different array, not just adding and removing values. To handle any possible change of value it should IMO look something like this:

Suggested change
// the watch will look to any changes in value to adjust selectedOptions for adding and removing values
immediate: true,
handler(newVal, oldVal) {
if (newVal && oldVal) {
if (newVal.length > oldVal.length) {
this.selectedOptions.push(newVal[newVal.length - 1]);
} else {
this.selectedOptions = this.selectedOptions.filter((option) =>
newVal.includes(option)
);
}
}
// the watch will look to any changes in value to adjust selectedOptions
immediate: true,
handler(newVal) {
if (newVal && JSON.stringify(newVal) !== JSON.stringify(this.selectedOptions)) {
this.selectedOptions = [...newVal];
}

@mehdimohammadijan
Copy link
Author

@mehdimohammadijan I would appreciate it if you also linked/published the code you used to make the screencast. Seeing what are the differences from how I use vue-multiselect would help me to figure out why it doesn't work (update the array value) for me.

Hello, thank you for your review.
inside child component mutating props is not allowed by vue, therefore another array of options is required in parent component to be sent as props to the component. inside the component it uses another collection state for dragging.
I am attaching the code, so that you can easily use it.
inside template
dataAndMethod

@slaweet
Copy link

slaweet commented Jan 14, 2022

Thank you for your reply @mehdimohammadijan

inside child component mutating props is not allowed by vue,

You cannot directly assign something to the value prop in multiselect component, but if you emit input event (as I suggested in #1504 (comment)), then v-model will take care of updating the value, as described in:
https://vuejs.org/v2/guide/components.html#Using-v-model-on-Components

therefore another array of options is required in parent component to be sent as props to the component. inside the component it uses another collection state for dragging.

Maybe it's not required. See https://github.com/shentao/vue-multiselect/pull/1504/files#r784726690.

I am attaching the code, so that you can easily use it.

Thanks that helps me a lot to understand how it was intended to be used.

<draggable
class="list-group"
tag="ul"
v-model="selectedOptions"
Copy link

Choose a reason for hiding this comment

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

I'm not too sure about this, but maybe selectedOptions wouldn't be needed if the v-model was split to v-bind:value and v-on:input. Relevant docs: https://vuejs.org/v2/guide/components.html#Using-v-model-on-Components

Suggested change
v-model="selectedOptions"
v-bind:value="value"
v-on:input="(v) => $emit('input', v)"

@mehdimohammadijan
Copy link
Author

I used value prop in v-model directly for draggablility, and additional array selectedOptions is removed. Now it is more clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants