Skip to content

Conversation

@evansiroky
Copy link
Contributor

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JSDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

fixes #610

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2020

Codecov Report

Merging #611 into dev will decrease coverage by 27.79%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              dev     #611       +/-   ##
===========================================
- Coverage   43.46%   15.67%   -27.80%     
===========================================
  Files         323      323               
  Lines       17603    16421     -1182     
  Branches     5368     4989      -379     
===========================================
- Hits         7652     2574     -5078     
- Misses       8676    11830     +3154     
- Partials     1275     2017      +742     
Flag Coverage Δ
#end_to_end_tests ?
#unit_tests 15.67% <0.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nager/components/validation/ServicePerModeChart.js 0.00% <0.00%> (-78.27%) ⬇️
lib/manager/components/validation/TripsChart.js 0.00% <0.00%> (-80.40%) ⬇️
lib/manager/components/HomeProjectDropdown.js 0.00% <0.00%> (-75.76%) ⬇️
lib/common/util/map-keys.js 25.00% <0.00%> (-75.00%) ⬇️
lib/editor/util/objects.js 14.89% <0.00%> (-72.35%) ⬇️
lib/editor/components/map/TextPath.js 0.00% <0.00%> (-70.00%) ⬇️
...b/manager/containers/ActiveFeedVersionNavigator.js 0.00% <0.00%> (-70.00%) ⬇️
lib/editor/components/timetable/HeaderCell.js 0.00% <0.00%> (-68.97%) ⬇️
lib/manager/components/DeploymentPreviewButton.js 15.78% <0.00%> (-68.43%) ⬇️
lib/editor/components/EditorSidebar.js 0.00% <0.00%> (-66.67%) ⬇️
... and 267 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 872d61d...67fd8bc. Read the comment docs.

if (column[mode] > 0) {
modes[mode] = true
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just do this check for modes with value > 0 in the filter below after the this._getData call? The way it currently is feels a little obfuscated. Alternatively, if you decide to keep it this way, the modes key could use a more descriptive name e.g., modesWithData or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up doing the check in the filter as you recommended.

Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Looks good, just a few non-blocking comments on code style.

@landonreed landonreed assigned evansiroky and unassigned landonreed Aug 31, 2020
@evansiroky evansiroky removed their assignment Sep 2, 2020
@binh-dam-ibigroup binh-dam-ibigroup merged commit 99c19b1 into dev Sep 9, 2020
@binh-dam-ibigroup binh-dam-ibigroup deleted the service-hours-chart-fix branch September 9, 2020 18:48
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.

ServicePerModeChart problem

5 participants