Improve touch controls for dash quality selector#4750
Merged
FreeTubeBot merged 8 commits intoFreeTubeApp:developmentfrom Mar 10, 2024
Merged
Improve touch controls for dash quality selector#4750FreeTubeBot merged 8 commits intoFreeTubeApp:developmentfrom
FreeTubeBot merged 8 commits intoFreeTubeApp:developmentfrom
Conversation
- Add `touchstart` event to quality button which toggles the `vjs-lock-showing` class used on other quality selectors - Call `this.handleClick` from touchstart (fixes issue with `e.target` not being correct) #239
github-actions
bot
added
the
PR: waiting for review
MarmadileManteater
requested review from
PikachuEXE and
absidue
and removed request for
absidue
PikachuEXE
previously approved these changes
Mar 7, 2024
(just like the other quality selectors do on mobile)
efb4f5ff-1298-471a-8973-3d47447115dc
approved these changes
Mar 7, 2024
Member
efb4f5ff-1298-471a-8973-3d47447115dc
left a comment
efb4f5ff-1298-471a-8973-3d47447115dc
left a comment
There was a problem hiding this comment.
LGTM
So idk if this is intentional but shouldnt the speed behave the same as the quality menu? If i tap on speed it will just move up instead of opening the menu for it, like for the qualities
VirtualBoxVM_swb0hDhHGU.mp4
Contributor
Author
|
I think it makes more sense to behave like the other quality controls (legacy & audio). Personally, I find the adjusting playback speed a little clunky. Its acceptable, but I don't think I would want that behavior for the quality selector. |
absidue
approved these changes
Mar 7, 2024
ChunkyProgrammer
approved these changes
Mar 10, 2024
github-actions
bot
removed
the
PR: waiting for review
PikachuEXE
added a commit
to PikachuEXE/FreeTube
that referenced
this pull request
Mar 13, 2024
…h-matching-videos * development: Fix handling of video published date in video lists (FreeTubeApp#4752) Translated using Weblate (Dutch) Translated using Weblate (Chinese (Simplified)) Bump lefthook from 1.6.4 to 1.6.5 (FreeTubeApp#4758) Bump marked from 12.0.0 to 12.0.1 (FreeTubeApp#4757) Bump electron from 29.1.0 to 29.1.1 (FreeTubeApp#4756) Translated using Weblate (Portuguese) Translated using Weblate (Portuguese (Brazil)) Improve touch controls for dash quality selector (FreeTubeApp#4750) Translated using Weblate (Serbian) Translated using Weblate (Spanish) Translated using Weblate (French) Translated using Weblate (German) Translated using Weblate (Estonian)
PikachuEXE
added a commit
to PikachuEXE/FreeTube
that referenced
this pull request
Mar 13, 2024
* development: (28 commits) Fix handling of video published date in video lists (FreeTubeApp#4752) Translated using Weblate (Dutch) Translated using Weblate (Chinese (Simplified)) Bump lefthook from 1.6.4 to 1.6.5 (FreeTubeApp#4758) Bump marked from 12.0.0 to 12.0.1 (FreeTubeApp#4757) Bump electron from 29.1.0 to 29.1.1 (FreeTubeApp#4756) Translated using Weblate (Portuguese) Translated using Weblate (Portuguese (Brazil)) Improve touch controls for dash quality selector (FreeTubeApp#4750) Translated using Weblate (Serbian) Translated using Weblate (Spanish) Translated using Weblate (French) Translated using Weblate (German) Translated using Weblate (Estonian) Translated using Weblate (Polish) Translated using Weblate (Basque) Translated using Weblate (Portuguese (Brazil)) Translated using Weblate (Portuguese (Brazil)) Translated using Weblate (Portuguese (Brazil)) Fix playlists database import and export not using the actual database format (FreeTubeApp#4664) ... # Conflicts: # src/renderer/views/Search/Search.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve touch controls for dash quality selector
Pull Request Type
Related issue
MarmadileManteater#239
Description
The dash quality selector does not respond well to touch input unlike the legacy and audio quality selectors. The
handleClickevent is being called, but the way that is being called on touch input causese.targetto reflect a different element than the one actually tapped on. This causes the quality selector to seem like it is ignoring user input when, it is just treating most taps as if they were on the current quality label (which causes the click handler to return immediately).This PR addresses this by adding a
touchstartevent to the button element which simply callshandleClickwith the appropriate target. Due to the fact that the quality selector is typically hidden and shown by ahoverpsuedo class, code was also added to the touchstart event to handle flipping thevjs-lock-showingclass.This PR also sets the max-block-size for the dash-quality selector on narrow displays in order to enable scrolling the list of qualities in the same way as the audio quality selector.
Screenshots
the touch responsiveness
the scrollability
Testing
dashis the preferred formatDesktop
Additional context
To be clear, it is possible to make the dash quality selector "work" (kind of) as is, but it requires "hovering" the button by hold tapping before making a quick shorter tap. Simply tapping once doesn't do anything most of the time.
note: I just noticed that the audio / legacy quality selector hides when a quality selection is made.
This implementation only hides the selector once theAfter adjusting the scroll-ability of the quality selector, I think it makes more sense to replicate the behavior of the existing audio selector and hide the selector after the selection is made.focusexitevent triggers. It may be preferable to hide the quality selector when a selection is made more like the other quality selectors on mobile, but I am divided since the dash quality selector tends to be tall enough that it needs to be able to be scroll-able.