-
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
Refactor and test missing value component #222
Conversation
- receives active category via prop (from parent) - receives missing values via getter - receives descriptions for values via getter
- mocked store mutation - listen to mocked mutation being called via spy - implement the mutation call in the component
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 a few discussion points, otherwise looks good to merge!
); | ||
} | ||
); | ||
it('handles lack of description gracefully', () => { |
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.
Do we want to standardize the field(s) we're using for mock store fields and functions? I'm referring specifically to how getters is defined at the top of this file as a separate field.
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.
Yeah, I was wondering this as well. I think there are some benefits to it:
- The stuff that can go in the component is prominently displayed up top
- Because this is a component test file, all tests are going to be for the same component. So we'll probably reuse the data structures in every test (except for minor changes that we can do ad hoc like in this test here)
But this was just the first thing I could think of. I agree it would make sense to be consistent in the future. what do you think?
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.
Yeah. Let's just pick something that mimics what we're emulating.
getters
, actions
(are we doing this?), mutations
, props
? plugins
?
Edit: What about the store? Is it state
? (This is even if we just use the getters.)
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.
Agreed. And I think state
is a good idea for passing just the store-state object. Should we put that in the wiki?
Did you want to change something about the tests here before merge too?
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.
Agreed. And I think state
is good Should we put that in the wiki?
Also: anything you'd like to change here in the test to match that?
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 the only remaining question is whether each of the above should be its own variable or if store-related ones (getters, mutations, actions, state) should be wrapped up into a store
object.
In this case, like:
const store = {
getters: { ... }
};
Otherwise, no changes.
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.
Ah, I see. If we do wrap it in a big store
object, we'll need to reference them when passing to the mount()
directive though, yes? What do you think is the benefit of either you think?
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 the mimicry of the actual app internals. For instance, props
would not be in the store
object. But that's about it.
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.
ah. OK. that makes sense to me!
Missing value component:
Component test:
closes #212