Skip to content
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

Modal Dialog #2668

Closed
wants to merge 36 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
bc5be3e
Add CloseButton component
misteroneill Oct 2, 2015
7e788c5
Add ModalDialog component
misteroneill Oct 2, 2015
aa86bea
Make sure components are included
misteroneill Oct 2, 2015
b14fb8a
Very basic modal styles
misteroneill Oct 2, 2015
0575a81
Modal also suppresses controls.
misteroneill Oct 2, 2015
a178f5c
Add linear-gradient mixin
misteroneill Oct 5, 2015
5041d8e
Remove unnecessary step
misteroneill Oct 5, 2015
46a178d
Fix typo
misteroneill Oct 5, 2015
a617e49
Add methods to fill/empty modal content
misteroneill Oct 5, 2015
b22f6a0
Support "closeability" on the modal
misteroneill Oct 5, 2015
d8edb19
Add fillWith, so fill uses option only
misteroneill Oct 5, 2015
bc84666
Add `content` method
misteroneill Oct 5, 2015
06ea1ca
Add 'fillAlways' option
misteroneill Oct 5, 2015
be7fb74
Reimplement `ErrorDisplay` as a subclass of `ModalDialog`
misteroneill Oct 5, 2015
6a4ac48
Allow strings as modal content
misteroneill Oct 6, 2015
855d590
tsml no longer needed
misteroneill Oct 6, 2015
7fb0166
Maybe fix PAM/IE8
misteroneill Oct 6, 2015
a556447
Add modalfill event
misteroneill Oct 7, 2015
1bf9d4c
Further flesh out fill/empty events
misteroneill Oct 7, 2015
467affd
Removals after feedback/discussion w/ @heff
misteroneill Oct 8, 2015
5e652b4
Remove CSS nesting and rename overlay-space.
misteroneill Oct 9, 2015
b372694
Add createModal method to Player
misteroneill Oct 9, 2015
370e2bc
Fixes to make tests work in IE8
misteroneill Oct 9, 2015
be00345
Quote map/array keys/values to prevent certain sass errors
misteroneill Oct 14, 2015
ca80d0a
Make close button consistent
misteroneill Oct 26, 2015
fc8cc9a
Add assertElHasClasses test helper
misteroneill Oct 26, 2015
c47b8f9
No need to depend on Events
misteroneill Oct 26, 2015
fdfd809
More explicit args for overloaded methods
misteroneill Oct 26, 2015
e97399c
Fix copy/paste error
misteroneill Oct 27, 2015
06e942f
Improve accessibility and element testing.
misteroneill Oct 27, 2015
a7def7c
Abstract content normalization to utils/dom.
misteroneill Oct 27, 2015
fa16f3d
Style tweaks and better close button handling.
misteroneill Oct 27, 2015
02947ed
Remove most of the error-display CSS
misteroneill Oct 27, 2015
cd48015
Use proper videojs-font version
misteroneill Oct 27, 2015
d786867
Make appendContent/insertContent better
misteroneill Oct 28, 2015
5e94d29
Restore error display 'X'
misteroneill Oct 28, 2015
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"object.assign": "^4.0.1",
"safe-json-parse": "^4.0.0",
"tsml": "1.0.1",
"videojs-font": "1.3.0",
"videojs-font": "1.4.0",
"videojs-ie8": "1.1.0",
"videojs-swf": "5.0.0-rc1",
"vtt.js": "git+https://github.com/gkatsev/vtt.js.git#vjs-v0.12.1",
Expand Down
8 changes: 7 additions & 1 deletion src/css/_utilities.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
@import "utilities/linear-gradient";

@mixin background-color-with-alpha($color, $alpha) {
background-color: $color;
background-color: rgba($color, $alpha);
Expand Down Expand Up @@ -94,11 +96,15 @@
order: $value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any ideas why github could be complaining about this property name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I noticed that. It's standard, though: https://drafts.csswg.org/css-flexbox-1/#order-property

}

%icon-default {
%fill-parent {
position: absolute;
top: 0;
left: 0;
width: 100%;
height: 100%;
}

%icon-default {
@extend %fill-parent;
text-align: center;
}
9 changes: 9 additions & 0 deletions src/css/components/_close-button.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
.video-js .vjs-control.vjs-close-button {
@extend .vjs-icon-cancel;
cursor: pointer;
height: 3em;
position: absolute;
right: 0;
top: 0.5em;
z-index: 2;
}
49 changes: 12 additions & 37 deletions src/css/components/_error.scss
Original file line number Diff line number Diff line change
@@ -1,48 +1,23 @@
.vjs-error-display {
display: none;
}

.vjs-error .vjs-error-display {
display: block;
position: absolute;
left: 0;
top: 0;
width: 100%;
height: 100%;
.vjs-error .vjs-error-display .vjs-modal-dialog-content {
font-size: 1.4em;
text-align: center;
}

.vjs-error .vjs-error-display:before {
color: #fff;
content: 'X';
font-family: $text-font-family;
font-size: 4em;
color: #fff;
/* In order to center the play icon vertically we need to set the line height
to the same as the button height */
line-height: 1;
text-shadow: 0.05em 0.05em 0.1em #000;
text-align: center /* Needed for IE8 */;
vertical-align: middle;
left: 0;

// In order to center the play icon vertically we need to set the line height
// to the same as the button height
line-height: 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just to signify an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, then I'll bring that back.

margin-top: -0.5em;
position: absolute;
left: 0;
text-shadow: 0.05em 0.05em 0.1em #000;
text-align: center; // Needed for IE8
top: 50%;
margin-top: -0.5em;
vertical-align: middle;
width: 100%;
}

.vjs-error-display div {
position: absolute;
bottom: 1em;
right: 0;
left: 0;
font-size: 1.4em;
text-align: center;
padding: 3px;

@include background-color-with-alpha(#000, 0.5);
}

.vjs-error-display a,
.vjs-error-display a:visited {
color: #66A8CC;
}
9 changes: 9 additions & 0 deletions src/css/components/_layout.scss
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,15 @@ body.vjs-full-window {
/* Hide disabled or unsupported controls. */
.vjs-hidden { display: none !important; }

// Visually hidden offscreen, but accessible to screen readers.
.video-js .vjs-offscreen {
height: 1px;
left: -9999px;
position: absolute;
top: 0;
width: 1px;
}

.vjs-lock-showing {
display: block !important;
opacity: 1;
Expand Down
13 changes: 13 additions & 0 deletions src/css/components/_modal-dialog.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.video-js .vjs-modal-dialog {
@extend %fill-parent;
@include linear-gradient(180deg, rgba(0, 0, 0, 0.8), rgba(255, 255, 255, 0));
}

.vjs-modal-dialog .vjs-modal-dialog-content {
@extend %fill-parent;

font-size: 1.2em; // 12px
line-height: 1.5; // 18px
padding: 30px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be in em as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same thing originally (was 2.5em), but increasing the padding as the font-size increases looks bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Also, rem is even less supported than em.

z-index: 1;
}
94 changes: 94 additions & 0 deletions src/css/utilities/_linear-gradient.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// These functions and mixins taken from:
//
// "Building a linear-gradient Mixin in Sass" by Hugo Giraudel
// http://www.sitepoint.com/building-linear-gradient-mixin-sass/
// http://sassmeister.com/gist/b58f6e2cc3160007c880
//

/// Convert angle
/// @author Chris Eppstein
/// @param {Number} $value - Value to convert
/// @param {String} $unit - Unit to convert to
/// @return {Number} Converted angle
@function convert-angle($value, $unit) {
$convertable-units: deg grad turn rad;
$conversion-factors: 1 (10grad/9deg) (1turn/360deg) (3.1415926rad/180deg);
@if index($convertable-units, unit($value)) and index($convertable-units, $unit) {
@return $value
/ nth($conversion-factors, index($convertable-units, unit($value)))
* nth($conversion-factors, index($convertable-units, $unit));
}

@warn "Cannot convert `#{unit($value)}` to `#{$unit}`.";
}

/// Test if `$value` is an angle
/// @param {*} $value - Value to test
/// @return {Bool}
@function is-direction($value) {
$is-direction: index((
'to top',
'to top right',
'to right top',
'to right',
'to bottom right',
'to right bottom',
'to bottom',
'to bottom left',
'to left bottom',
'to left',
'to left top',
'to top left'
), $value);
$is-angle: type-of($value) == 'number' and index('deg' 'grad' 'turn' 'rad', unit($value));

@return $is-direction or $is-angle;
}

/// Convert a direction to legacy syntax
/// @param {Keyword | Angle} $value - Value to convert
/// @require {function} is-direction
/// @require {function} convert-angle
@function legacy-direction($value) {
@if is-direction($value) == false {
@warn "Cannot convert `#{$value}` to legacy syntax because it doesn't seem to be an angle or a direction";
}

$conversion-map: (
'to top' : 'bottom',
'to top right' : 'bottom left',
'to right top' : 'left bottom',
'to right' : 'left',
'to bottom right' : 'top left',
'to right bottom' : 'left top',
'to bottom' : 'top',
'to bottom left' : 'top right',
'to left bottom' : 'right top',
'to left' : 'right',
'to left top' : 'right bottom',
'to top left' : 'bottom right'
);

@if map-has-key($conversion-map, $value) {
@return map-get($conversion-map, $value);
}

@return 90deg - convert-angle($value, 'deg');
}

/// Mixin printing a linear-gradient
/// as well as a plain color fallback
/// and the `-webkit-` prefixed declaration
/// @access public
/// @param {String | List | Angle} $direction - Linear gradient direction
/// @param {Arglist} $color-stops - List of color-stops composing the gradient
@mixin linear-gradient($direction, $color-stops...) {
@if is-direction($direction) == false {
$color-stops: ($direction, $color-stops);
$direction: 180deg;
}

background: nth(nth($color-stops, 1), 1);
background: -webkit-linear-gradient(legacy-direction($direction), $color-stops);
background: linear-gradient($direction, $color-stops);
}
2 changes: 2 additions & 0 deletions src/css/video-js.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
@import "components/layout";
@import "components/big-play";
@import "components/button";
@import "components/close-button";

@import "components/menu/menu";
@import "components/menu/menu-popup";
Expand Down Expand Up @@ -35,3 +36,4 @@
@import "components/subtitles";
@import "components/adaptive";
@import "components/captions-settings";
@import "components/modal-dialog";
28 changes: 28 additions & 0 deletions src/js/close-button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import Button from './button';
import Component from './component';

/**
* The `CloseButton` component is a button which fires a "close" event
* when it is activated.
*
* @extends Button
* @class CloseButton
*/
class CloseButton extends Button {

constructor(player, options) {
super(player, options);
this.controlText(options && options.controlText || this.localize('Close'));
}

buildCSSClass() {
return `vjs-close-button ${super.buildCSSClass()}`;
}

handleClick() {
this.trigger({type: 'close', bubbles: false});
}
}

Component.registerComponent('CloseButton', CloseButton);
export default CloseButton;
59 changes: 32 additions & 27 deletions src/js/error-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,53 +2,58 @@
* @file error-display.js
*/
import Component from './component';
import * as Dom from './utils/dom.js';
import ModalDialog from './modal-dialog';

import * as Dom from './utils/dom';
import mergeOptions from './utils/merge-options';

/**
* Display that an error has occurred making the video unplayable
* Display that an error has occurred making the video unplayable.
*
* @param {Object} player Main Player
* @param {Object=} options Object of option names and values
* @extends Component
* @extends ModalDialog
* @class ErrorDisplay
*/
class ErrorDisplay extends Component {
class ErrorDisplay extends ModalDialog {

/**
* Constructor for error display modal.
*
* @param {Player} player
* @param {Object} [options]
*/
constructor(player, options) {
super(player, options);

this.update();
this.on(player, 'error', this.update);
this.on(player, 'error', this.open);
}

/**
* Create the component's DOM element
* Include the old class for backward-compatibility.
*
* @return {Element}
* @method createEl
* This can be removed in 6.0.
*
* @method buildCSSClass
* @deprecated
* @return {String}
*/
createEl() {
var el = super.createEl('div', {
className: 'vjs-error-display'
});

this.contentEl_ = Dom.createEl('div');
el.appendChild(this.contentEl_);

return el;
buildCSSClass() {
return `vjs-error-display ${super.buildCSSClass()}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely not relevant to this PR but wouldn't it be awesome if Component did the class ${super.buildCSSClass}$ for us if we just returned 'class' from this function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. My concern with that sort of implicit behavior is that it's hard to override if you don't want it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a separate method.

mixinCSSClass() {
  return 'vjs-error-display'; // gets combined with superclass classNames
}

buildCSSClass() {
  return 'vjs-error-display'; // overrides, identical behavior to today
}

Of course, it doesn't look like buildCSSClass gets implicitly called anywhere (like in Component#createEl), so maybe it would make sense to auto-inherit like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe a separate method. Anyway, completely separate from this PR :)

}

/**
* Update the error message in localized language
* Generates the modal content based on the player error.
*
* @method update
* @return {String|Null}
*/
update() {
if (this.player().error()) {
this.contentEl_.innerHTML = this.localize(this.player().error().message);
}
content() {
let error = this.player().error();
return error ? this.localize(error.message) : '';
}
}

ErrorDisplay.prototype.options_ = mergeOptions(ModalDialog.prototype.options_, {
fillAlways: true,
uncloseable: true
});

Component.registerComponent('ErrorDisplay', ErrorDisplay);
export default ErrorDisplay;
Loading