Refactor user skills service #1011
Conversation
app/utils/skills-list.js
Outdated
| case 'user-skill': | ||
| return _matchWithRelationship(item, skill, relationship, 'user'); | ||
| default: | ||
| return undefined; |
There was a problem hiding this comment.
Not sure what to return for default here.
app/utils/skills-list.js
Outdated
| function _matchForModel(model, item, skill, relationship) { | ||
| switch(model) { | ||
| case 'user-skill': | ||
| return _matchWithRelationship(item, skill, relationship, 'user'); |
There was a problem hiding this comment.
This switch was best way I could think of. Would appreciate other suggestions.
| get | ||
| } = Ember; | ||
|
|
||
| let skillsList = { |
There was a problem hiding this comment.
Not sure of a better convention here for this.
There was a problem hiding this comment.
I love that this is split out into a regular module, rather than a mixin/service.
As for the conventions, did you mean you are not sure if you should be exporting just an object literal? For this use case, where you are exporting 2 functions, I don't really see a problem with it.
you could export them both as named exports and let things that are importing it choose what they import. Or even make the default export be an object literal of the two functions still.
joshsmith
force-pushed
the
refactor-user-skills-input
branch
from
29a3e6e
to
8f8aac5
Compare
| @@ -12,15 +13,13 @@ export default Component.extend({ | |||
| tagName: 'li', | |||
|
|
|||
| matched: notEmpty('userSkill'), | |||
| userSkills: service(), | |||
| userSkillsList: service(), | |||
There was a problem hiding this comment.
Did you look into seeing if we can inject the type of skills list this is?
There was a problem hiding this comment.
As you said earlier, I think we'll loop back to this once we have to expand out to other cases.
joshsmith
force-pushed
the
refactor-user-skills-input
branch
from
82c800a
to
28c88ef
Compare
There was a problem hiding this comment.
@joshsmith I left a few small comments. Other than those I like this change.
|
|
||
| contains(skill) { | ||
| let userSkills = get(this, 'userSkills'); | ||
| return skillsList.contains(userSkills, skill); |
There was a problem hiding this comment.
Has contains been deprecated in favour for Enumerable#includes?
There was a problem hiding this comment.
Oh, it has. This contains is kind of a custom thing. It's really doing array-like operations on something that's not an array. Maybe that's confusing?
There was a problem hiding this comment.
ah yes - I definitely read this as if it was Enumerable#contains at first. But that makes sense now.
| get | ||
| } = Ember; | ||
|
|
||
| let skillsList = { |
There was a problem hiding this comment.
I love that this is split out into a regular module, rather than a mixin/service.
As for the conventions, did you mean you are not sure if you should be exporting just an object literal? For this use case, where you are exporting 2 functions, I don't really see a problem with it.
you could export them both as named exports and let things that are importing it choose what they import. Or even make the default export be an object literal of the two functions still.
| function _matchForModel(model, item, skill, relationship) { | ||
| switch (model) { | ||
| case 'user-skill': | ||
| return _matchWithRelationship(item, skill, relationship, 'user'); |
There was a problem hiding this comment.
@joshsmith I am not sure what the plan is down the line, or if the other model types would leverage the same _matchWithRelationship function. If they did you could do a mapping object for relationship name. Otherwise, this seems like a valid use-case for a switch statement.
| return (itemRelationshipId === relationshipId) && (itemSkillId === skillId); | ||
| } | ||
|
|
||
| export default skillsList; |
There was a problem hiding this comment.
Is there any chance we can add some documentation around this module? I'm a big fan of documentation in util modules.
There was a problem hiding this comment.
Sure. May want to add some unit tests for this, too.
Attempt to refactor user skills service to make it generalizable to any type of skill relationship.