Fix frost toggle click handler #438
Conversation
| @@ -0,0 +1,19 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
No actual change in this guy, just reformatting from .eslintrc to .eslintrc.js b/c javascript > json 😉
package.json
Outdated
| @@ -46,7 +46,7 @@ | |||
| "ember-cli-notifications": "^4.1.6", | |||
| "ember-cli-test-loader": "^1.1.1", | |||
| "ember-cli-uglify": "^1.2.0", | |||
| "ember-code-snippet": "^1.8.0", | |||
| "ember-code-snippet": "ciena-blueplanet/ember-code-snippet#remove-browserify", | |||
There was a problem hiding this comment.
I did raise an issue with ember-code-snippet to make them aware of the issue, but in the meantime, we can just use our fork.
package.json
Outdated
| @@ -98,5 +98,8 @@ | |||
| "ember-spread": "^1.1.1", | |||
| "fs-extra-promise": "^0.4.0", | |||
| "npm-install-security-check": "^1.0.0" | |||
| }, | |||
| "pr-bumper": { | |||
| "coverage": 92.72 | |||
There was a problem hiding this comment.
This will help ensure that code coverage doesn't drop as the result of a PR
|
Approved |
|
Looks like a few tests need to be updated to deal with true/false CPs in the toggle being readOnly - I'd probably recommend removing those unit tests since we've agreed that integration tests should cover components in a black-box approach |
Sounds good. I was gonna refactor the unit tests, but just updating the integration tests to make sure all the use-cases are covered sounds much better. |
|
@sglanzer Worth taking another look, this is a bit different since you approved last. It now implements the |
|
Hold on, just found another bug, closing this while I fix it. Will re-open once I have the new tests/fix in place. |
|
OK @sglanzer I think it's ready for review again. Sorry for the hold-up. |
|
Hmm.
Looks like I need to add a couple more tests. Coming right up. |
| @@ -53,45 +58,44 @@ export default Component.extend(SpreadMixin, PropTypeMixin, FrostEventsProxyMixi | |||
| }, | |||
|
|
|||
| getDefaultProps () { | |||
| const falseLabel = this.get('falseLabel') | |||
| const trueLabel = this.get('trueLabel') | |||
| const validTypes = ['string', 'number', 'boolean'] | |||
There was a problem hiding this comment.
I'm honestly not sure why we would care about the type of the label - I know this was there in the previous code, I just can't think of any reason for it
There was a problem hiding this comment.
Me either. The label should always be a string, and spec'd that way in propTypes in my opinion, but I didn't want to change functionality in this PR, just clean up the code :)
| _trueValue (trueValue, _trueLabel) { // eslint-disable-line | ||
| return trueValue || _trueLabel | ||
| _trueValue (trueValue, _trueLabel) { | ||
| return (trueValue === undefined) ? _trueLabel : trueValue |
There was a problem hiding this comment.
Probably another case where we can't change this due to backwards compatibility, but it probably makes more sense to default to true instead of using the label as the value (typically we would use the value as a label, not the other way around)
| _falseValue (falseValue, _falseLabel) { // eslint-disable-line | ||
| return falseValue || _falseLabel | ||
| _falseValue (falseValue, _falseLabel) { | ||
| return (falseValue === undefined) ? _falseLabel : falseValue |
There was a problem hiding this comment.
Same obviously - likely we can't do anything about these comments, but maybe we can make an LTS issue pointing to this PR so that we can cleanup in the next release
|
Approved - all good from my end - re-kicked a few failed builds (3 passed, it's probably just travis) |
|
Could be the code coverage still. I'll take a look and either adjust the baseline (since I'm only enabling it in this PR anyway) or add some missing tests elsewhere to increase coverage. I did make sure I was covering toggle well. The only bit not covered was some code I couldn't figure out how to reach, but didn't want to delete for backwards compatibility. |
|
Raised an issue to track the comments we can't address currently #440 |
|
@job13er yeah, you're getting code coverage drops, but only in 2 of the builds (used to be 3) - we may need to be careful about that strategy |
|
Yeah, should only be checking in one build, I'll fix it. |
|
@job13er - As a reference, here is what I did to limit the checking to one build: |
|
Something screwy is going on. I shouldn't be getting different coverage numbers for different builds. We don't have conditional code based on ember/noe version (to the best of my knowledge) |
|
Yeah, I'm not sure what that's about - maybe we only want to run coverage numbers against the LTS release build? |
| @@ -0,0 +1,15 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This should let us only check coverage on the same build where we bump.
|
Approved |
|
I'm stuck behind bunsen builds, so it may be a while :( |
|
Changes Unknown when pulling 38d0bc9 on job13er:fix-frost-toggle-click-handler into ** on ciena-frost:master**. |
|
Changes Unknown when pulling 44d3c17 on job13er:fix-frost-toggle-click-handler into ** on ciena-frost:master**. |
|
Approved |
|
Woooh, finally got it merged - good stuff 👍 |
This project uses semver, please check the scope of this pr:
Since I made a few, unrelated changes in this PR, I tried to group my commits in a meaningful way. By looking at the diffs per commit, you should be able to understand what changes were needed for what effect. If it's unclear, just ask.
CHANGELOG
[email protected][email protected](using a pinned[email protected]so as to avoid overwritingrequireand breakingember-cli-code-coverage)onTogglecallback which is called with the new value whenever toggledonClickcallback, which wasn't handling raw functions very well when going through the event proxytrueValue/falseValuewhentrueLabel/falseLabelare set