Replace multi-line comment with single line comments #2827

nick11703 · 2015-11-19T17:51:54Z

When using Compass to compile the sass files, mulit-line comments are
not stripped in the output, causing malformed CSS output.

Example

.video-js {
  /* display:inline-block would be closer to the video el's display:inline
   * but it results in flash reloading when going into fullscreen [#2205]
   */
  display: block;
  /* Make video.js videos align top when next to video elements */
  vertical-align: top;
  box-sizing: border-box;
  color: #fff;
  background-color: #000;
  position: relative;
  padding: 0;
  /* Start with 10px for base font size so other dimensions can be em based and
  easily calculable. */
  font-size: 10px;
  line-height: 1;
  /* Provide some basic defaults for fonts */
  font-weight: normal;
  font-style: normal;
  /* Avoiding helvetica: issue #376 */
  font-family: Arial, Helvetica, sans-serif;
  -webkit-user-select: none;
  -moz-user-select: none;
  -ms-user-select: none;
  user-select: none;
  /* Fix for Firefox 9 fullscreen (only if it is enabled). Not needed when
  checking fullScreenEnabled. */
}

Chrome's interpretation of the CSS

When using Compass to compile the sass files, mulit-line comments are not stripped in the output, causing malformed CSS output.

gkatsev · 2015-11-19T18:23:13Z

I'm okay with changing the single line comments to use // instead of /* */. I'm not a fan of changing multi-line comments to use single-line comments. Seems like this is a bug that compass should fix instead of us fixing our code.

nick11703 · 2015-11-19T18:38:41Z

Compass will ignore stripping multi-line comments and output them in the final CSS. The problem really come from multi-line comments in CSS class definitions. Single line comments are stripped by compass during compilation. Can we agree to replace multi-line comments inside CSS definitions with single line comments? If so, I can update the pull request.

Updated to include a link to Sass documentation on comments.

gkatsev · 2015-11-19T19:24:02Z

Can you open an issue against compass regarding this? They should strip out comments that won't be valid in pure css. I'll pull it in then with the change for those.
Sounds good?

misteroneill · 2015-11-19T19:30:08Z

It isn't Compass doing it, it's Sass. I don't think it's a bug in Sass, either. It may be a Chrome bug because it sees that commented out CSS property as an actual CSS property.

misteroneill · 2015-11-19T19:31:53Z

I did a test. The following SCSS has its comment preserved, but shows no issues in the Chrome dev tools:

.video-js {
  /*
  baz baz baz baz
  bar bar bar bar
  */
  display: block;
}

But this shows the behavior in the attached screenshot:

.video-js {
  /*
  display: inline
  baz baz baz baz
  bar bar bar bar
  */
  display: block;
}

gkatsev · 2015-11-19T19:32:43Z

Yeah, I just looked at videojs's css and the comments are preserved. Though, it's still working fine.

nick11703 · 2015-11-19T19:33:37Z

I don't believe it's a Compass issue either. Sass states it will ignore multi-line comments. I wonder if it's chrome picking up the display:inline-block in the comment.

nick11703 · 2015-11-19T19:37:52Z

Either way, if it's a Sass, Compass or browser issue, is it necessary to output the comments in the compiled CSS? They remain in the .scss files for the developer. To help make the web faster it would reduce final output by allowing single line comments to be stripped.

misteroneill · 2015-11-19T20:00:27Z

FWIW, the comments do get stripped in dist/video-js.min.css, which is really what people should be using.

gkatsev · 2015-11-19T20:16:18Z

I think I know what the exactly issue here.
Since comments aren't valid inside blocks, chrome looks at the block line by line to see whether it's valid or not and so if you have a multi-line comment, some of the things that are commended out could be interpreted incorrectly.

I looked through the changes and I think so of the things can just be moved outside of the blocks. The things that should stay inside the block can be changed to single line comments. I'll go through and make comments about it.

gkatsev · 2015-11-19T20:17:52Z

src/css/components/_progress.scss

-  /* If we let the font size grow as much as everything else, the current time tooltip ends up
-     ginormous. If you'd like to enable the current time tooltip all the time, this should be disabled
-     to avoid a weird hitch when you roll off the hover. */
+  // If we let the font size grow as much as everything else, the current time tooltip ends up


move this out of the block

gkatsev · 2015-11-19T20:20:12Z

@nick11703 if you make the changes discussed, I'll pull it in. Thanks!

nick11703 · 2015-11-19T20:20:56Z

@gkatsev No problem. I'll work on that soon. Thanks.

gkatsev · 2015-12-03T22:07:29Z

@nick11703 hey, did you get a chance to go through and update the comments? (Looks like this also needs a rebase).

nick11703 · 2015-12-04T14:20:44Z

@gkatsev Sorry. Got caught up with the holidays. It's in my plans for today.

# Conflicts: # src/css/components/menu/_menu-popup.scss

nick11703 · 2015-12-04T14:51:16Z

@gkatsev This should be good to go now. Let me know if you need anything else.

gkatsev · 2015-12-04T16:30:04Z

LGTM

dmlap · 2015-12-07T18:10:13Z

LGTM

@gkatsev

* upstream/stable: (479 commits) v5.4.4 @gkatsev switched to use custom vtt.js from npm. closes videojs#2905 v5.4.3 @gkatsev updated options customizer and github-release options. closes videojs#2903 v5.4.2 @gkatsev updated grunt-release config. closes videojs#2900 v5.4.1 @gkatsev added chg- and github- release for next releases. closes videojs#2899 v5.4.0 @gkatsev added ability to release next tag from master. closes videojs#2894 @gkatsev added nullcheck for cues in updateForTrack. Fixes videojs#2870. closes videojs#2896 @chemoish emulated HTMLTrackElement to enable track load events. closes videojs#2804 @gkatsev added a Player#reset method. Fixes videojs#2852. closes videojs#2880 @nick11703 changed multiline comments in sass with single-line comments. closes videojs#2827 @gkatsev added Player#tech. Fixes videojs#2617. closes videojs#2883 @misteroneill updated videojs-ie8 to 1.1.1. closes videojs#2869 v5.3.0 @imbcmdth added sourceOrder option for source-first ordering in selectSource. closes videojs#2847 @forbesjo updated formatTime to not go negative. closes videojs#2821 v5.2.4 ...

Replace multi-line comment with single line comments

954be37

When using Compass to compile the sass files, mulit-line comments are not stripped in the output, causing malformed CSS output.

gkatsev reviewed Nov 19, 2015
View reviewed changes

Nicholas Fantozz added 3 commits December 4, 2015 09:23

Updates requested comment changes

9ae1828

Merge remote-tracking branch 'videojs/master'

194b501

# Conflicts: # src/css/components/menu/_menu-popup.scss

Removes conflict and adds single line comment

4964876

gkatsev added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Dec 4, 2015

gkatsev mentioned this pull request Dec 4, 2015

5.4.0 release proposal #2882

Closed

7 tasks

gkatsev closed this in 9e3a7b6 Dec 7, 2015

gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace multi-line comment with single line comments #2827

Replace multi-line comment with single line comments #2827

nick11703 commented Nov 19, 2015

gkatsev commented Nov 19, 2015

nick11703 commented Nov 19, 2015

gkatsev commented Nov 19, 2015

misteroneill commented Nov 19, 2015

misteroneill commented Nov 19, 2015

gkatsev commented Nov 19, 2015

nick11703 commented Nov 19, 2015

nick11703 commented Nov 19, 2015

misteroneill commented Nov 19, 2015

gkatsev commented Nov 19, 2015

gkatsev Nov 19, 2015

gkatsev commented Nov 19, 2015

nick11703 commented Nov 19, 2015

gkatsev commented Dec 3, 2015

nick11703 commented Dec 4, 2015

nick11703 commented Dec 4, 2015

gkatsev commented Dec 4, 2015

dmlap commented Dec 7, 2015

Replace multi-line comment with single line comments #2827

Replace multi-line comment with single line comments #2827

Conversation

nick11703 commented Nov 19, 2015

gkatsev commented Nov 19, 2015

nick11703 commented Nov 19, 2015

gkatsev commented Nov 19, 2015

misteroneill commented Nov 19, 2015

misteroneill commented Nov 19, 2015

gkatsev commented Nov 19, 2015

nick11703 commented Nov 19, 2015

nick11703 commented Nov 19, 2015

misteroneill commented Nov 19, 2015

gkatsev commented Nov 19, 2015

gkatsev Nov 19, 2015

Choose a reason for hiding this comment

gkatsev commented Nov 19, 2015

nick11703 commented Nov 19, 2015

gkatsev commented Dec 3, 2015

nick11703 commented Dec 4, 2015

nick11703 commented Dec 4, 2015

gkatsev commented Dec 4, 2015

dmlap commented Dec 7, 2015