-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix various UI things in our paella integration #1208
Conversation
Basically all changes look good to me and are a clear improvement! Code looks good as well. I have only two remarks:
I know this was discussed in the issue, but it's worth noting that this is also an accessibility concern. Making them less opaque obviously reduces contrast, and makes it hard to see for some people. Tobira does not yet have a high contrast mode, otherwise we could at least improve the situation in that mode. So yeah, maybe Olaf and David can reconsider whether the buttons in the current form (without this PR) are really too distracting.
I think I would prefer if the quality selection thing could always be there, even if there is only one quality. Then at least users can see the quality they are currently watching. I am not sure if it's possible to convince Paella of that though... |
Testing this on a large screen, for once. A certain improvement can be seen, thanks. However, adding the "Seek video to the previous/next slide" icons was not a request from my end, it was a fear I expressed because it leaves us with eight (!) icons within the video canvass. Which makes me give up on the idea of making them more opaque/"invisible", also in conjunction with the hover-over effect, the different black and grey backgrounds, Firefox adding its thing with a different grey, and the accessibility concerns Lukas mentions. Is there a chance therefore of hiding icons individually the way you added the icons for slide navigation?
+1. Which kind of kills the settings menu, does it? The additional "CC" probably shouldn't be hidden there. |
also, re this comment (sorry for not adressing this yet): #1196 (comment): |
These buttons will now be smaller and use less margin when the video container is smaller than 400px. They will also be less opaque overall and only be completely opaque when directly hovered over.
6cf413e
to
c17c49b
Compare
This menu holds the options for resolution, captions and video layout to make the player appear less cluttered.
I have opened polimediaupv/paella-core#368 to address the issue of open submenus not being fixed to their trigger button, and polimediaupv/paella-skins#6 to figure out what to do with the vertical lines in the progress bar. |
This allows us to always show the quality selector, even if there is only one available resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Clear improvements on all fronts!
I'm personally not the biggest fan of the bold time-font, but I agree that given the icons and other text, it looks better. Maybe at some point we can make everything a bit thinner, but at least now it's consistent.
For reference, I just checked: it is possible, by changing the Tobira configuration (specifically paella_plugin_config = """{
"es.upv.paella.frameControlButtonPlugin": { "enabled": false }
}""" This disables the button that lets you see all slide previews. Sadly, the buttons that you, Olaf, wanted to remove most of all -- e.g. the "enlarge" button hovering over the video -- don't seem to be controlled by plugins? Or at least I don't see which ones are responsible for that so I can't disable it right now. But if we are really interested we could dig deeper or ask the Paella devs. |
This mainly does two things:
Also:
Things mentioned in #1196 but missing here:
-> this is not technically possible. Users need to disable it themselves in their browser.
-> I believe this needs to be fixed in paella-skins as any custom changes are always overwritten by that.
Oh and I also noticed the thing @dagraf mentions in #1196 (comment): Open submenus are not fixed but move with the screen when scrolling up or down. This also needs to be fixed in paella.
Closes #1196
Marked as draft bc I still need to figure out how to hide the settings menu when there are no menu items 🙈