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

Add basic drag & drop functionality to color instances #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rocbear
Copy link

@rocbear rocbear commented Feb 17, 2022

Hey @pabueco, thanks for the great tool! I was playing around with it and felt a drag and drop interaction for the color mixer would be handy. If it's something you're interested in including please check out this PR. It includes a simple version that relies on the HTML API (see gif below).

drag

@pabueco
Copy link
Owner

pabueco commented Feb 17, 2022

Hey @rocbear, thank you very much for your contribution. I really like the drag and drop functionality.
The only issue is that toggling the color picker does no longer work. When the colored area of a <ColorCard /> is clicked, a color picker should be shown. I guess that it might have something to do with the HTML drag API in combination with the listener on the card:

on:click|self={() => (isColorPickerVisible = !isColorPickerVisible)}

Maybe this could also be fixed by only allowing the user to drag the colors with a handle, like an additional icon next to the x-icon, instead of the whole element. But this might make the implementation a bit more complex.

If you fix this issue (in the way that you prefer) I will happily merge this.
Thank you! ✌️

@rocbear
Copy link
Author

rocbear commented Feb 17, 2022

@pabueco ah right, thanks! I'll look into it and update the PR tomorrow.

@vercel
Copy link

vercel bot commented Feb 23, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pabueco/hue-tools/AFdxgyufV92o2TbcFuzseMmT5duM
✅ Preview: https://hue-tools-git-fork-rocbear-dragndrop-pabueco.vercel.app

@rocbear
Copy link
Author

rocbear commented Feb 23, 2022

@pabueco I think the problem came from Svelte not being able to tell how to keep track of the items by key, which I'd changed to colorInstance where it was previously index. I realized that doesn't matter because there's no way to uniquely identify the colors without adding some kind of ID.
I'm new to Svelte so I'm not sure if my most recent changes are ok in practice, but they seem to work. I just wrapped the #each block in a #key pragma that watches changes to a variable which I update whenever the list order is changed. It feels messy anyway because there are no visual cues to show how the list will be reordered when a drop is made.

Maybe a 'reverse order' or 'shift forward/back' button would be a better option?

@pabueco
Copy link
Owner

pabueco commented Feb 23, 2022

So, first of all it's nice that it works now!

I also don't have that much experience with svelte, but your approach seems sensible and works well.
On the other hand it might also be a good idea to just generate a random id in the Color model and use that as the key in the loop.

I agree that it does not feel perfect, but it's a lot better than having no way to reorder the colors. And as long as a feature like this does not negatively impact the experience of the app in general I'm happy to have a working solution that has room for improvement in the future.

While I generally prefer buttons (up, down, reverse, etc.) to reorder elements because they are usually easier to implement and more consistent across devices, I don't really want to add more icons to the color cards. An option would be to add a dropdown to each color and move the delete and reorder functionalities in there. But that also doesn't sound that great to me.

So I think as long as everything is working correctly your current solution is just fine.
If you want to refine it a bit, the behaviour seen in this repl feels quite nice and seems quite easy to implement.

Thanks for your work!

@pabueco
Copy link
Owner

pabueco commented Feb 25, 2022

@rocbear Just a quick info. I just merged this PR which added a unique id to each color instance and is used in the each loop of the mix tool. So this should allow you to remove the hacky solution with the update time.

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.

2 participants