-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement getCategories store getter #284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small issues regarding naming/styling and good to go
components/category-select-table.vue
Outdated
@@ -39,14 +40,14 @@ | |||
|
|||
...mapGetters([ | |||
|
|||
"categories", | |||
"getCategories", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the sake of simplicity - why include get
in the name? Is this something we just went with from the Miro? The code using the returned value from the getter should not care that it's interacting with a getter.
Since categories
is now a more complex field in the refactor, and we really want the names of the categories here how about naming this getter something like categoryNames
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can take a page out of the old Java OOP best practices and use get(for getters) and set(for mutators) prefixes. It has a nice ring to it but I agree on the 2nd point maybe rename it to getCategoryNames?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like the combo here. @jarmoza: I like that the methodname tells you in easy language what it does. I haven't found any official recommendations so far, but I like the thinking in this issue: vuestorefront/vue-storefront#2069
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@surchs Seems like a good basis for a standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'm tracking this here now: #287. For the refactor I would say we make the vuex method names consistent as we implement them. So I will leave categoryClasses
alone for now, even though it should also start with get
to be consistent.
store/index-refactor.js
Outdated
@@ -2,11 +2,16 @@ | |||
import Vue from "vue"; | |||
|
|||
export const state = () => ({ | |||
columnToCategoryMapping: {} | |||
columnToCategoryMapping: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should alphabetize object keys if we don't have a functional grouping for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @jarmoza. Didn't we have something setup to complain / fix this issue as part of the linting? Should we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! 🎉
vuex getters should start with `get`.
nice... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @surchs!
Closes #254