Closed Tyrritt closed 5 months ago
netlify[bot]
commented
6 months ago | Name | Link |
|---|---|
| Latest commit | 282bb0870450219d4b19b4361b88480c120a0914 |
| Latest deploy log | https://app.netlify.com/sites/ytify/deploys/66464358d64d900008217322 |
n-ce
commented
6 months ago Thanks for the wonderful work. I was trying this myself as you can tell by the commented code but was failed by the sticky behaviour of the elements. Then I thought something must be wrong with the queuelist container they are in so i postponed it to a refactor.
First things I noticed about this PR
dblclick event, is it a standard event or custom?<queue-stream-item> can you explain why? You could have used <stream-item> with a <div> to wrap it as well.About the drag handle, my point on implementing one is that it increases the width of the list items which means we have to reduce the width of the original list item to maybe a 85% for drag handle to take 15% space. The icon for which could be https://remixicon.com/icon/draggable. Since we were going to wrap it into a div anyway, we could fit the drag handle into that.
<div>
<stream-item></stream-item>
<i class="ri-draggable"></i>
</div>
Tyrritt
commented
6 months ago Thanks for the feedback, I really appreciat it! For the sake of keeping this short, let me go over your points one by one:
git reset one of my failed attempts. I know what causes this and have a fix readydblclick is standard, see https://w3c.github.io/uievents/#event-type-dblclickqueued-stream-itemI hope I'll be able to make some changes in a few days, I am currently occupied with other things.
n-ce
commented
6 months ago Thanks for the feedback, I really appreciat it! For the sake of keeping this short, let me go over your points one by one:
- Right, I made this branch before I created the PR for Play Next. I shall rebase soon
- Huh I thought I fixed this locally but it must've got eaten when I
git resetone of my failed attempts. I know what causes this and have a fix readydblclickis standard, see https://w3c.github.io/uievents/#event-type-dblclick- I originally made the new component to be able to edit it and try things out without breaking stuff in other lists. I do like your idea to warp it in a div and have an additional handle component in the wrapper. I'll implement it when I can and will revert the addition of
queued-stream-item- I do not know what lighthouse is, will look into it
- This is what I was worried about and tried to describe iny initial comment. I have no idea what goes wrong and causes it.
- I will add the handle icon, thanks for the quick explanation in the wiki!
I hope I'll be able to make some changes in a few days, I am currently occupied with other things.
Hey take your time! I'm currently experimenting with replacing lit components with solid-js tsx components, if all things go well it might help reduce the complications with this PR. Thank you for your work on improving this project.
Tyrritt
commented
6 months ago Quick update from me, I still dont have much time
stream-item in a div and make the handle a child of said div, the handle will appear outside the "video box". This probably looks bad00826b0, However, see the top bullet pointn-ce
commented
6 months ago Quick update from me, I still dont have much time
- If we wrap
stream-itemin adivand make the handle a child of said div, the handle will appear outside the "video box". This probably looks bad- Issue with the Enqueue All button gets resolved if I revert
00826b0, However, see the top bullet point- I still have no clue whatsoever why on some machines / devices / ... you cannot move the items
Please rebase from the oembed branch. I'll try to attempt the rest. https://github.com/n-ce/ytify/blob/50d72438eff32ac9d1ef7f583ddee79c1a21a1c2/src/components/StreamItem.tsx#L62-L90 we are going to insert the handle just beside the metadata div, it should be hidden by default so that when the class is toggled it's visible, this would be the case for queue (and user imported playlists), a lot of things are broken right now but with continued hard work and persistence a polished and refactor could be made in time
Tyrritt
commented
6 months ago Rebase to oembeddone, I'll have a look at the actual code too soon and see where I can support.
Thanks for your help!
n-ce
commented
6 months ago I have integrated the draggable icon with the stream item to be active dynamically on queue using the draggable property passed to it.
n-ce
commented
5 months ago I removed the dblclick references and used the drag handle instead which seemed to have fixed the issues. I'm going ahead with the merge now. Thanks for this very necessary feature implementation.
I tried my hand at making queue items movable via drag & drop provided by SortableJS. Keep in mind JS is not my forte and I pretty much only do simple stuff in projects that i find useful myself.
Some parts, like the drag handle content / icont are still WIP, but I need some input.
It works fine with my testing on desktop browsers (FF, Chrome, other Chromium-based: Brave, Edge), but as soon as I switch to Responsive Design Mode it stops working. I'm stuck here and need some input (tag this PR Help Needed?) Over the last few days I have tried the following things to no avail:
I did however find, that removing the ease-out transition in line 222 in src/stylesheets/style.css at least prevents the dragged item from snapping / glitching back to its original position all the time. Dropping the item on a different location does not fire the appropriate events (OnMove, OnUpdate, OnEnd). The element is not actually being moved in the DOM.
If there no solutions or obvious things I have missed come up here, I should probably open an issue about this in SortableJS.
EDIT: i just tested the deployed preview on my actual phone running Bromite on LineageOS and it seems to work just fine. Maybe it is just an issue with the Responsive Design Mode in the Desktop browser. Can somebody else please confirm if it works on their mobile device too?