ALM-Rangers / Countdown-Widget-Extension

Counts down to a configurable moment in time.
https://marketplace.visualstudio.com/items?itemName=ms-devlabs.CountdownWidget&utm_roundup=github
Other
11 stars 16 forks source link

Henry/conditional formatting #32

Closed henrybeen closed 6 years ago

henrybeen commented 6 years ago

Thank you for your pull request! By completing this pull request, you agree to the Contributing License Agreement.

Changes proposed in this pull request:

@ALM-Rangers/countdownwidget

afbeelding

jessehouwing commented 6 years ago

Review status: 0 of 8 files reviewed at latest revision, all discussions resolved.


CountdownWidget/CountdownWidget/src/configuration.ts, line 146 at r1 (raw file):

      colorSettings.color = (settings && settings.backgroundColorHoursColor)
          ? settings.backgroundColorHoursColor
          : "red";

Why not make this a settings.defaultBackgroundColor?


CountdownWidget/CountdownWidget/src/countdownWidget.ts, line 175 at r1 (raw file):

      $title.text(this.currentSettings.name);

      $countDownBody.css("background-color", this.renderValues.backgroundColor);

This requires 'unsafe-inline' for Content Security Policy. It's probably better to manipulate the DOM element directly instead of overriding the css value.


CountdownWidget/CountdownWidget/static/css/app.css, line 44 at r1 (raw file):

foreground-picker


CountdownWidget/CountdownWidget/static/css/app.css, line 70 at r1 (raw file):

}

.indented {

Personally, don't like CSS classes that have the name of what they do instead of the name of what they're intended for.

What is getting indented here. Why is that so, is there a better name?


CountdownWidget/CountdownWidget/static/css/app.css, line 76 at r1 (raw file):

}

.inputwrapper {

input-wrapper? or inputWrapper?


Comments from Reviewable

jessehouwing commented 6 years ago

FYI few remarks. Plus, seems that master is again ahead of this branch. So a new rebase/merge is needed.


Reviewed 8 of 8 files at r1. Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

jessehouwing commented 6 years ago

Review status: all files reviewed at latest revision, 5 unresolved discussions.


CountdownWidget/CountdownWidget/package.json, line 42 at r1 (raw file):

  },
  "dependencies": {
    "@types/jasmine": "^2.5.53",

Seems to be a change that was not really intentional... Just moved the lines.


CountdownWidget/CountdownWidget/package.json, line 44 at r1 (raw file):

    "@types/jquery": "^2.0.34",
    "@types/q": "0.0.32",
    "@types/jasmine": "^2.5.53",

Seem


Comments from Reviewable