Skip to content

[IMPAC-654] IE: Javascript errors on widget settings and display#401

Merged
cesar-tonnoir merged 8 commits intomaestrano:release/v1.6.1from
rheasunshine:bug/654-ie-js-err-settings
Sep 18, 2017
Merged

[IMPAC-654] IE: Javascript errors on widget settings and display#401
cesar-tonnoir merged 8 commits intomaestrano:release/v1.6.1from
rheasunshine:bug/654-ie-js-err-settings

Conversation

@rheasunshine
Copy link
Copy Markdown
Contributor

@cesar-tonnoir

-Fixed some js method errors in IE with lodash (there are inconsistencies with this throughout the project that should be fixed at some point)
-Allow settings changes in expense to turnover widget
-Fixed salaries summary and workforce summary widgets
-Made input range bars (globally) visible in IE

@rheasunshine rheasunshine changed the title Bug/654 ie js err settings [IMPAC-654] IE: Javascript errors on widget settings and display Sep 6, 2017
Copy link
Copy Markdown
Contributor

@cesar-tonnoir cesar-tonnoir left a comment

Choose a reason for hiding this comment

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

A few minor changes, other than that: looks good.
thanks for the lodash fixes

$scope.displayAccount = ->
$scope.updateSettings(false).then ->
w.format()
$timeout ->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do you need a $timeout here?
the timeout should be applied to the execution of w.format(), not to the definition of the method (alos, , 0 is optional:

$scope.displayAccount = ->
  $scope.updateSettings(false).then ->
    $timeout(-> w.format())

}
input[type="range"] {
height: 20px;
height: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

have you tested this does not impact the other widgets that already use a range input?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes the other widgets are good to go in both IE and chrome.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

suggestedMin: 0
suggestedMax: 100
maxTicksLimit: 5
suggestedMin: 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can even refactor it to:

options = {
  scales:
    yAxes: [
      {
        ticks:
          suggestedMin: 0
          suggestedMax: 100
          maxTicksLimit: 5
      }
    ]
  showXLabels: false
  pointDot: false
  currency: '%'
}

} No newline at end of file
.analytics .widget-item {
.content {
height: auto !important;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, this should be fixed another way (without using !important).
Plus, be careful there: you are applying height: auto !important; to .analytics .widget-item .content => it will be applied to all the widgets, not only salaries summary...

@rheasunshine
Copy link
Copy Markdown
Contributor Author

@cesar-tonnoir Hey fixed all but the Range input... Should work everywhere. Need to make sure I've covered all the widgets it is used in.. otherwise other fixes are pushed

@cesar-tonnoir cesar-tonnoir changed the base branch from 1.6 to release/v1.5.7 September 18, 2017 06:09
@cesar-tonnoir cesar-tonnoir changed the base branch from release/v1.5.7 to release/v1.6.1 September 18, 2017 06:10
} No newline at end of file
.analytics .widget-item {
.content {
height: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary - will remove

.expandable-filterable-widget();
.analytics .widget-item {
.content {
height: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here

}
.chart-container {
max-height: 125px;
max-height: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not required

@cesar-tonnoir cesar-tonnoir merged commit 2147ed1 into maestrano:release/v1.6.1 Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants