Closed ArturoManzoli closed 1 month ago
Latest updates from @ES-Alexander changes request: 1, 2, 5, 6 - Done;
3 and 4- This grid/variable individual styling and preview is planned to be implemented when porting this screen to the next UI;
7- There is an auto-scroll feature when you drag a chip to the top of the view area. Too many scrollbars usually degrades the UI harmony and inserting them in this case also did. Anyway, your suggestion was added to the next UI port.
8- The standard text for non named missions is 'cockpit', that goes onto the filename when downloading a video, for example. Now the 'Mission name' tag will show 'Cockpit' instead of simply not showing anything. Getting the auto generated mission name is not practical for now, but it has been added as suggestion for the next UI port.
Hey @ES-Alexander, thanks for taking the time to review this PR and for providing such detailed and valuable feedback. :+1:
As a MacOS user, things might be presented a little differently for you (and this is very useful), such as the hotspot for auto-scrolling being outside the view area and the number inside the selector for the shadow size being cut off.
About 2- Is nice to reset the variable positions between UI updates. Their references may change and odd behavior may occur.
Updates: 1, 2, and 6 - Fixed. 7 - The target area for auto-scrolling changes slightly depending on the browser and OS. Following your suggestion, the area was increased to the upper 40% of the view.
Hey @ES-Alexander, thanks for taking the time to review this PR and for providing such detailed and valuable feedback. 👍
No worries - glad I can help find and resolve some issues before they end up in a release :-)
1, 6 - Nice 👍 2, 7 - Seemingly unchanged from my last comment?
https://github.com/bluerobotics/cockpit/assets/25898329/3f3d8d6b-9839-4a8d-94cd-f56749c4f685
2 is definitely some kind of bug, 7 might just be an OS difference or something (I don't think that one's a big deal, just not quite as nice as it could be - I think it's ok to merge this once 2 is fixed if 7 is hard to track down, although it might just be an element layering thing in terms of where pointer events are registered - not sure 🤷♂️ ).
For 2, I'm pretty sure the issue is caused by line 307 - that likely needs a filter for "unless it's in the mission variables" :-)
For 2, I'm pretty sure the issue is caused by line 307 - that likely needs a filter for "unless it's in the mission variables" :-)
Nice catch. The filtering was being triggered too late on line 317. Moved to line 307.
2 is definitely some kind of bug, 7 might just be an OS difference or something (I don't think that one's a big deal, just not quite as nice as it could be - I think it's ok to merge this once 2 is fixed if 7 is hard to track down, although it might just be an element layering thing in terms of where pointer events are registered - not sure 🤷♂️ ).
About 7, I think I saw it working (but a little laggy) on the first seconds of the video you posted. It's working on non safari browsers and it shouldn't be different on safari based.. but Apple...
If you change the line:
document.getElementById('draggable-container')!.scrollIntoView({ behavior: 'smooth' })
to
document.getElementById('draggable-container')!.scrollIntoView({ behavior: 'instant' })
the screen will simply center on the display grid with no scrolling. Is it a better solution?
Check the regular behavior on Chrome: https://github.com/bluerobotics/cockpit/assets/14910201/44925375-4507-464c-b62e-6f9cfaee29ba
2 is fixed :+1:
About 7, I think I saw it working (but a little laggy) on the first seconds of the video you posted.
Nah, that's when the chip goes into one of the grid cells, which then scrolls the page to bring that full cell into view.
It's working on non safari browsers and it shouldn't be different on safari based.. but Apple...
I'm on Chrome, so not a Safari issue.
If you change the line:
document.getElementById('draggable-container')!.scrollIntoView({ behavior: 'smooth' })
todocument.getElementById('draggable-container')!.scrollIntoView({ behavior: 'instant' })
the screen will simply center on the display grid with no scrolling. Is it a better solution?
I tried this and it does work but it's super jarring, so I wouldn't say it's a better solution.
Check the regular behavior on Chrome: ...
The behaviour in your video looks as I'd expect/hope mine to work :-)
I played around a little and found out that if I flip the threshold check on line 389 I get the desired behaviour, so it seems the Y direction in my Chrome is upwards (graph style), whereas I suppose in yours it's downwards (image/matrix style)? I tried a different browser and the original threshold comparison worked as expected, so I think we can just assume it's a bug in my Chrome version or something and leave it for now.
621