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 touch supported class for touch supporting devices #5663
Add touch supported class for touch supporting devices #5663
Conversation
Added a new test to check that the class is *NOT* applied when the device does *not* support touch and fixed the already existing unit test that checked for the contrary.
Huh, I wonder why this is was commented out. |
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.
So, looking at the PR that added the commented out code, it seems like it was added commented out (https://github.com/videojs/video.js/pull/672/files#diff-3b0266ff1c037b289ec624ab25b0272eR48). I assume it was because we wanted to do other stuff related to the class before adding it, like changing the control size based on whether touch is enabled or not, but that gets tricky.
Only issue I can think of is that this class should be updated if/when touch support is removed. For example, if you move your window from one monitor to another and the new monitor doesn't have touch support, the class should be removed. Unfortunately, I am not sure if there's an event or something we can listen to to know this. So, maybe it's fine for this PR and that can be a future enhancement.
Thoughts?
test/unit/player.test.js
Outdated
|
||
const player = TestHelpers.makePlayer({}); | ||
|
||
assert.ok(player.el().className.indexOf('vjs-touch-enabled') === -1, 'touch-enabled classname not added'); |
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.
would be better to use assert.equal()
here
assert.ok(player.el().className.indexOf('vjs-touch-enabled') === -1, 'touch-enabled classname not added'); | |
assert.equal(player.el().className.indexOf('vjs-touch-enabled'), -1, 'touch-enabled classname not added'); |
test/unit/player.test.js
Outdated
@@ -632,7 +632,23 @@ QUnit.test('should add a touch-enabled classname when touch is supported', funct | |||
|
|||
const player = TestHelpers.makePlayer({}); | |||
|
|||
assert.ok(player.el().className.indexOf('vjs-touch-enabled'), 'touch-enabled classname added'); | |||
assert.ok(player.el().className.indexOf('vjs-touch-enabled') !== -1, 'touch-enabled classname added'); |
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.
and assert.notEqual() here
assert.ok(player.el().className.indexOf('vjs-touch-enabled') !== -1, 'touch-enabled classname added'); | |
assert.notEqual(player.el().className.indexOf('vjs-touch-enabled'), -1, 'touch-enabled classname added'); |
Huh, I wonder why the netlify is breaking... |
@gkatsev From what we understood, the idea of issue #2999 would be to have a way to selectively style the player, based on the device supporting touch or not. As such, we found that it might be useful to have a CSS class applied to it. I have done some investigation and it appears that there is no easy way to detect if a window has been dragged to another monitor. The best idea I could have about how to do that would be something like using About the test assertion styles, we attempted to follow the style of the other unit tests, and thus wrote it like that. However, I agree with the suggestions and can change the code if that is best. |
@gkatsev About the netlify deploy, it appears to be something in the npm package installation section, so before any Is there any way to reattempt the deploy preview? That might fix it. We tested the new code against |
Restarting the netlify build, we'll see if it fixes it. If it doesn't, it's not a big deal, since it's just to make it easier for us to test with. Yeah, especially since given that we don't use the class currently, just uncommenting it is fine. We should probably add another issue to track the window move or at least add a comment in above adding the class saying that it won't update if touch support is modified after the player is initalized. |
Some of these tests are pretty old, we try and update them to make it better if the change is pretty small, like this one. |
@gkatsev Updated the tests as requested. On the subject of the tracking of the window, I have done some further research and have not found anything relevant to go off of (not even any experimental browser API in MDN), so what I suggest is the creation of the relevant issue, seeing as comments documenting the behaviour can be missed (as ocurred in this case) |
Thanks! Yeah, an issue for this would be great. I think adding it in the comment (perhaps with a link to the issue) would also be good. |
@gkatsev Created issue #5683 as requested, and added it to the comment in player.js, before adding the class. Would this PR be ready to be accepted? We are working on this open source project in the context of a university course, and it would be beneficial if this PR could be approved (should not even need to be merged, just have its changes accepted). |
Thanks! This PR LGTM, though, we require two approvals for outside contributors, though, I don't see it not being accepted. That course sounds interesting, can you tell me more about it? |
Also, I just realized why the netlify build is failing. The base branch is against a version of master that still uses event-stream 3.3.6 which had the security issue. If you're able to rebase this against (or merge in) master it should fix the netlify build. |
@gkatsev Thanks a lot! We are from the Faculty of Engineering of the University of Porto (FEUP), and the course is called ESOF, which is a portuguese acronym for Software Engineering. (This is a link to the course page in english) In this course we learn about common and historical software development pitfalls (we work on a research paper about a historical software bug), software development methodologies (such as Scrum and Kanban, on which we also prepared a paper and a class presentation) and in the third part of the course each class chooses an open source project to contribute to. That is the reason you might have seen some more activity in terms of outside contributors lately - for example #5662 is also from my group, and #5629 is from another group in my class. |
@gkatsev We can try and merge the |
@gkatsev Done! That was easier than expected, I didn't know that updating a fork from an upstream was that simple haha Let's hope this fixes the netlify deploy then |
Yeah, it's not too bad. |
This PR is good to go now when we start merging for the next minor release. We may want to make a couple patch releases first. |
BTW, can I ask why Video.js was chosen? |
Congrats on merging your first pull request! 🎉🎉🎉 |
Description
Fixes issue #2999
Programatically adding
vjs-touch-enabled
CSS class to Player when the device has touch support.This enables custom player styling for touch devices as mentioned in the linked issue
Specific Changes proposed
Adding
vjs-touch-enabled
CSS class in touch devices (based on the value ofbrowser.TOUCH_ENABLED
)Fixed previous unit test for class application (which always passed) and added a unit test for the opposite case (make sure the class is not added when the device does not support touch)
Requirements Checklist