Modal Dialog #2668
Modal Dialog #2668
Conversation
|
This is a WIP, but I'm opening a PR to start gathering feedback and spark discussion as to whether or not this belongs in video.js proper. |
misteroneill
force-pushed
the
modal-dialog
branch
from
a7d18b4
to
369f3d8
Compare
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 369f3d86c6a886ed2b54e2cbfd0d41ede8d8ea74 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: a7d18b45223b141b929b6096ae22a82a7d5d3d7c (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: d2988f2bfd8550e2bfda3f27aa27d407e770d556 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 3a08ded21093aed904f3b3e7bb258ef65805563f (Please note that this is a fully automated comment.) |
|
|
||
| $gradient-start: rgba(0, 0, 0, 0.8); | ||
| $gradient-stop: rgba(255, 255, 255, 0); | ||
| background: $gradient-start; |
|
This touches on our strategy for UI components in plugins moving forward, which doesn't necessarily need to be solved here but will impact it. I think we need to strongly define how plugins should be accessing UI components to use/extend them. It's totally inefficient for plugins to be pulling in entire copies of video.js as a dependency to get at a component. One step forward might be to break out UI components into their own repos/projects, which is something we've talked about either way, but even then it seems really inefficient to have a plugin compile Component along with itself, when another copy of Component will already be in the page with Video.js. It sounds like peer dependencies are taking a big step forward in npm@3. Are they a viable option yet? As far as this issue is concerned, if we go the route of having copies of components compiled with plugins, then I don't think it makes sense to pull this into core. If only because we might be moving all other components out. If we go the route of saying plugins should use the copies included in video.js then a modal does seem like a root-level UI component, and we could probably justify it just because we could use it for the internal error display. If it weren't for that we'd be deciding if Video.js should include UI components just for what it uses internally, or if it should include additional general purpose UI components for the sake of plugins. I think I would lean towards the former in that case. |
This is only really an issue if npm plus a packing system like browserify is not used. npm will auto-dedupe packages and if the end-user uses videojs and plugins from npm, you only include one version of video.js.
One solution is to release each component into a separate repo. Then plugins can require just what they need. Basically, we can have a concept of component-plugins and have all the built-in videojs components be those themselves. This probably would need a pretty major rework to the API and how things work. But might be a good thing in the end. Maybe this plus redefined plugin API can be The Next Big Thing™ for videojs.
Ultimately, we can't really support everything. For this, we might need to find the happy path (I recommend browserify+npm) and support that path fully. That doesn't mean we shouldn't support other ways too, but it could mean that payloads end up slightly larger because there isn't a good way of doing dedupes and stuff like that if we're supporting globals for everything. However, I guess plugins can be smarter and have different builds. One browserify build that requires everything and assumed the end-user also uses browserify (or webpack). One global build that assumes none of the dependencies are available and/or one global build that assumes all the dependencies will be available in the global scope.
Yes, seems like in npm 3, peer dependencies are finally not a hassle. However, with npm 3 also deduping everything by default, peer deps aren't as useful because plugins can just depend on videojs and the end-user who uses browserify themselves, can also just require them and then npm will dedupe videojs so you only end up with one videojs in your tree. Also, npm is working on better front-end package managing. Maybe we should talk to them about what they're working on and what our needs are.
I think we should update the internal error display to use this modal dialog. |
|
It seems like the big concern here is that builds for plugins will have video.js bundled with them. I agree that should be avoided! I think @gkatsev is right that npm/browserify solves this problem. Although, I do wonder how it would work with multiple versions of video.js in its dependency tree (e.g. if plugin-a depends on video.js@5.1.x and plugin-b depends on video.js@5.0.x)? I agree with both of you that the internal error display could be updated to use this; it justifies the inclusion in video.js proper in my mind. One of my main concerns was having UI components in video.js that were only there for plugin authors. The flip side of that introduced another concern: having a plugin whose only purpose was to provide blank-slate UI components. Sort of a meta-plugin. Perhaps that's not a bad thing. Any opinions on that? |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: a9c21fdeb3cabf07843a7d3752989590806210a6 (Please note that this is a fully automated comment.) |
|
npm de-duping is great but it's not a complete answer unfortunately
If we were a full app framework we might be able to force stricter development pipelines on plugin authors and users, but video.js is a widget that's going to be dropped into a lot of existing pipelines, and using the independently compiled versions of plugins is gonna be the norm for a while. If peer dependencies are better now it looks to me like they could support the best of both worlds. Are there still reasons why we wouldn't want to go that route? |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 5058f8bae9496b83ad1a8961c2a5df59a002d42a (Please note that this is a fully automated comment.) |
|
I don't think anyone was suggesting forcing people into an npm/browserify workflow. Maybe I'm not clear on the problem. Plugins already depend on video.js (implicitly as a global); so, I'm not sure how this work would change that or how calling |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: c1cfd051d158b05cdfd81966a5febf565a596657 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 0fb1999bbff661a1b479ea922db8c545ba76dfbc (Please note that this is a fully automated comment.) |
misteroneill
force-pushed
the
modal-dialog
branch
from
0fb1999
to
83a68f5
Compare
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 83a68f5b98941888d153c614e14c0f68e0702b09 (Please note that this is a fully automated comment.) |
|
Check out videojs-dock as an example. It doesn't rely on Video.js as a global, and if you were to compile that for users looking to include it as a standalone js file next to video.js (not using browserify), it would include another copy of Video.js. I may have jumped ahead, but that's what I assumed we meant by the npm workflow that doesn't rely on peer dependencies. |
|
It actually does currently rely on videojs as a global. I use browserify-shim to shim it in so the code looks like it can be required. Also, not sure why I didn't use es6 module syntax there. |
|
Oh wow, that's cool. I stand corrected. |
| * disposed as soon as it's closed, which is useful for one-off | ||
| * modals. | ||
| * | ||
| * @param {Boolean} [options.fillAlways=false] |
There was a problem hiding this comment.
Part of me wonders if this should be the default case and the "fill once" case should be the special case. The reason I went this way is that "fill once" is more conservative (fewer DOM changes, etc).
There was a problem hiding this comment.
At first glance Modal feels really complicated for what I think of as a box that displays text. I feel like it'd be nice if we could simplify it but I also don't understand all the use cases going into it. Do you think you could document the different use cases you're trying to solve for with the different options here?
There was a problem hiding this comment.
Yeah, I can elaborate on it more in the PR description.
Both fillImmediately and openImmediately can probably go away. They were added pretty early on, but it's just as easy to do var m = new ModalDialog(); m.open();.
The slug option may also be a candidate for removal; though, it makes modifying the HTML class on a per-instance basis hard. Sub-classes can use buildCSSClass.
Another candidate for removal is the fillWith() function. Of course, it does allow developers to bypass the content() step entirely, which may be convenient, and it's only a few extra lines of code.
There was a problem hiding this comment.
Another candidate for change would be to rename the open/close methods to show/hide (overriding/expanding on those methods of Component).
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: ffc410b9c8c0c7174f80d361e817aa260442da37 (Please note that this is a fully automated comment.) |
|
Also, what I was kind of suggesting is that we target global and browserify packaging. This should hopefully be relatively simple to do by using browserify to write the plugins. Then with various plugins like browserify-shim, we can produce two builds of the plugin. One of the builds is the "happy path" I was describing above which uses browserify and depends on video.js and that is what you get when you |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: d4792b8c6f523b3ee13e5beb5a81ee5a7659cf07 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 1f366f63ffc354bcbf5efd0078c48a56b6b2411d (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 50c5241d927ee59066ac43e38cdf87524aee96f7 (Please note that this is a fully automated comment.) |
|
+1 @gkatsev, interested in seeing that. I currently have to shim video.js with webpack as well. However, video.js itself could also benefit from module loader packaging. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: d44816fc6edddc131165fa3535346926bcaa582e (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 6fcfd90dfb3cc7aaeb3b64d7e34796309c0abbbf (Please note that this is a fully automated comment.) |
This makes testing for the presence of multiple classes on a component a little cleaner/simpler.
This improves modal accessibility by allowing a description element and some additional updates to ARIA attributes. Also, this removes the previously-added `assertElHasClasses` in favor of a more comprehensive `assertEl` method on the test helpers object.
Also adds a number of methods to the DOM utility and lots of tests for those. Simplifies the modal dialog component correspondingly.
misteroneill
force-pushed
the
modal-dialog
branch
from
3438abd
to
a7def7c
Compare
|
@gkatsev Alright, this is ready for further review - particularly the last 4 commits. Note particularly the changes to |
|
@pam retry |
|
|
||
| font-size: 1.2em; // 12px | ||
| line-height: 1.5; // 18px | ||
| padding: 30px; |
There was a problem hiding this comment.
should probably be in em as well.
There was a problem hiding this comment.
I thought the same thing originally (was 2.5em), but increasing the padding as the font-size increases looks bad.
There was a problem hiding this comment.
Makes sense. Also, rem is even less supported than em.
| if (typeof value === 'string' && /\S/.test(value)) { | ||
| return document.createTextNode(value); | ||
| } | ||
| }).filter(value => value); |
There was a problem hiding this comment.
The map will only return nodes or undefined, so this removes the undefineds since they can't be inserted into the DOM.
|
Makes sense to fix the focus stuff later. I guess what we would want is to capture it only if the focus is already inside the player area but not otherwise? Something to think about. |
|
|
||
| player.controls(true); | ||
| this.hide(); | ||
| this.el().setAttribute('aria-hidden', 'true'); |
There was a problem hiding this comment.
I was wondering about what value this attribute should get since boolean attributes in HTML are checked via presence rather than value so it's customary to set their value to their name but seems like ARIA specifically looks for the value true.
Just wanted to call that out for others that care :)
There was a problem hiding this comment.
Good point @gkatsev!
ARIA doesn't follow HTML syntax, in fact it's a language with its own syntax which can be used within HTML (but also within other languages). The fact that HTML booleans are either false through their absence or true through presence which can be denoted as, e.g. selected="selected" is an SGML vs XML thing, whereas ARIA values may be booleans or true, false or undefined (as well as true/false/mixed!), so the syntax was defined differently.
Setting aria-* attributes otherwise is a syntax violation, but it's a tricky one to catch since HTML validators generally look at static pages rather than what the JavaScript is doing.
|
I wonder why pam hasn't run on this after the rebase... |
|
I think she gave up. 😈 |
| } | ||
|
|
||
| .vjs-error .vjs-error-display:before { | ||
| content: 'X'; |
There was a problem hiding this comment.
was this removed on purpose?
I would prefer to not change the look of the error display too much. Or at least, we still need something to make it noticeable as an error display and not just an uncloseable modal dialog.
There was a problem hiding this comment.
Yeah. It looked totally broken with that style - it duplicates a lot of the style of the modal dialog.
I don't know what the "X" was about (I was guessing a "close" icon/button).
There was a problem hiding this comment.
I think just to signify an error.
There was a problem hiding this comment.
Ah, then I'll bring that back.
|
LGTM |
|
I know this isn't the place for this but there's no where else to turn. Would someone mind posting some example code of how to use this new function? I can't find a single example on the internet. |
|
@sethborg No worries. There are ModalDialog API docs, but I agree that a more prose-based document would be helpful. I'll add an issue to create one. The absolute easiest way to create a modal is the // Get a reference to your player.
var player = videojs.getPlayers().yourPlayerId;
player.createModal('Hello, world!');Note that if you want to populate it with HTML - and you probably do - you can pass a DOM element reference or a function that creates one: player.createModal(function () {
var content = document.createElement('h1');
content.textContent = 'Hello, world!';
return content;
});Beyond this, the |
|
Oh this creates a modal dialog on the video player area. I was under the impression this allowed me to create a lightbox over the whole webpage with the video player in it. The API should clear that up. |
This PR introduces two new components to video.js:
CloseButtonandModalDialogwith tests.The general idea is to strike a balance between assumptions of how a modal should work - e.g., you want a "close" button or you only want to define the contents once - and flexibility.
At Brightcove, there are five plugins (both open and closed source) which individually define some kind of a modal. Every one in slightly and subtly different; so, part of the goal is that this can work for all those while anticipating potential needs of other plugin authors.
Much of the flexibility comes from adding hooks to important lifecycle events: fill, empty, open, close. This allows things like event binding to happen when users want - for example, it might make sense to bind events on
"modalfill"so we are sure the DOM is in place.