BaseService: make serviceData and badgeData explicit and declarative #1451
Conversation
Generated by 🚫 dangerJS |
| colorA: makeColor(overrideColorA), | ||
| }; | ||
| const color = makeColor(overrideColorB || serviceColor || defaultColor || 'lightgrey'); | ||
| setBadgeColor(badgeData, color); |
There was a problem hiding this comment.
This duplicates code in makeBadgeData but ultimately will replace it. I don't think it makes sense to reuse that function here; it just makes the code harder to reason about and more roundabout.
To make this even cleaner, we could rewrite some of these helper functions and/or change the interface to makeBadge.
| @@ -24,6 +24,29 @@ class DummyService extends BaseService { | |||
| } | |||
|
|
|||
| describe('BaseService', () => { | |||
| describe('_makeBadgeData', function () { | |||
There was a problem hiding this comment.
We have all cases with text tested here and that's great! Would you like to add tests for other properties? There's a lot of code connected to colors which isn't tested here.
There was a problem hiding this comment.
Yea, I'd love to add more tests, though maybe we can merge this and tag-team the tests. There is plenty of untested functionality here.
…1543 This merges the `node-8` branch. The heavy lift was by @Daniel15 with refactoring from me and a patch by @RedSparr0w. * New API for registering services (#963) * Disable Node 6 tests on node-8 branch (#1423) * BaseService: Factor out methods _regex and _namedParamsForMatch (#1425) - Adjust test grouping - Rename data -> queryParams, text -> message * BaseService tests: Use Chai (#1450) * BaseService: make serviceData and badgeData explicit and declarative (#1451) * fix isValidStyle test (#1544) * Run tests in Node 9, not Node 6 (#1543)
Based on feedback in #1425.
@platan @RedSparr0w