-
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: handle controlled terms as objects #374
Refactor: handle controlled terms as objects #374
Conversation
Need to use the reduce option in vue-select to handle objects as options and identify them based on their identifier while showing the label to the user
No functional changes, just passing the expected data, now that terms are objects and not literals any more.
No functional changes, but updated examples to pass around identifiers instead of human readable labels as before. Also changed the third test to actually rewrite the same value as asserted.
@surchs Interesting! Will take a look. We should have some of these integration PRs for the annotation page being merged in today/early next week, so I'll consider best order for the merges. Could be yours here should be first! |
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 Just one question and maybe 1-2 light changes. I think this should go in before my PR with Arman that he's reviewing now.
Let me know what you think.
|
||
// Setup | ||
cy.mount(annotCategorical, { | ||
computed: Object.assign(store.getters, { getCategoricalOptions: () => (p_column) => [], |
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 Can you explain the reasoning behind this Object.assign
and also the null return value of getSelectedOption
mock?
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.
the reasoning behind this Object.assign
this just creates a new Object of store.getters
where the getCategoricalOptions
and getSelectedOptions
attributes are overwritten with what's shown above without changing the original store
object defined at the beginning. An alternative way to do this would be to add the beforeEach
hook to define the store
and then directly change the store
object in the setup of this test store.getters.getCategoricalOptions = ...
. The main goal here is to change how these getters behave for this specific test from how they are defined at the beginning.
the null return value of getSelectedOption mock
as mentioned in the test name, the goal here is to just check whether the component is able to deal with a null
return value from the getter. The reason for this is that we now expect the getter to return an object where it previously returned a string literal - and I expect the (yet to be implemented) getter to return null
if no selection has been made so far. So we want to be sure that the component can handle 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.
this just creates a new Object of store.getters where the getCategoricalOptions and getSelectedOptions attributes >are overwritten with what's shown above without changing the original store object defined at the beginning. An >alternative way to do this would be to add the beforeEach hook to define the store and then directly change the store >object in the setup of this test store.getters.getCategoricalOptions = .... The main goal here is to change how these >getters behave for this specific test from how they are defined at the beginning.
Okay for now. I'll convert this to the beforeEach
when I merge these changes with my PR changes - which I have done already in my new version of this test file. One of the reasons I asked about the Object.assign
is I've seen some oddness with store.getters
functions being shown as undefined by Cypress. The solution was to redefine the store
in a beforeEach
before each test.
as mentioned in the test name, the goal here is to just check whether the component is able to deal with a null return >value from the getter. The reason for this is that we now expect the getter to return an object where it previously >returned a string literal - and I expect the (yet to be implemented) getter to return null if no selection has been made >so far. So we want to be sure that the component can handle that.
👍
mutations.selectCategoricalOption(store.state, "female", "column1", "F"); | ||
mutations.selectCategoricalOption(store.state, "female", "column1", "Female"); | ||
mutations.selectCategoricalOption(store.state, "https://example.org/female", "column1", "F"); | ||
mutations.selectCategoricalOption(store.state, "https://example.org/male", "column1", "F"); |
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.
Selecting 'F' for 'male'?
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, that wouldn't be a correct choice. But the syntax works the other way around. It's:
selectCategoricalOption(p_state, p_optionValue, p_columnName, p_rawValue) {
so here I'm selecting "male" for "F" when I have previously selected "female" for "F", thus overwriting the previous value. Before we just assiged the same selection to two different raw values and so we weren't testing that we can overwrite something.
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.
Gotcha
@surchs Approved to merge. As I explain above, I'll handle any merge conflicts with my PR that will be merged next. |
Closes #371
{label: "human readable label", identifier: "https://example.org/vocab/termID"}
Overall the changes are limited, because the code can mostly just handle terms as objects already.
Two things to note:
selectCategoricalOption
to overwrite the stored value as described in the testNote: