refactor: xna2bids schema#104
Conversation
… updating I think I need to switch back to emits so the v-bind works as expected
|
Visit the preview URL for this PR (updated for commit 4ddc60a): https://bnc-script-generator--pr104-refactor-xna2bids-sc-ycemc70j.web.app (expires Fri, 04 Mar 2022 21:08:41 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
eldu
left a comment
eldu
left a comment
There was a problem hiding this comment.
Looking good and more deletions! I added in a question and bat signaled mary 😆
| }, | ||
| fieldType: { | ||
| type: String, | ||
| required: false, |
There was a problem hiding this comment.
I think required: false is the default but it's alright that you include this!
| removeSession: 'xnat2bids/popSession', | ||
| }), | ||
| updateValue(index, key, val) { | ||
| this.value[index][key] = val |
There was a problem hiding this comment.
You're updating the prop directly here from the child. Are you experiencing any issue here? I would have thought that you needed to emit the value and then set the prop in the parent component.
Maybe @mcmcgrath13 ?
There was a problem hiding this comment.
Yeah, directly updating a prop can cause an infinite render loop. Generally emitting and updating the parent is the safer bet.
| fieldProps() { | ||
| const optProps = {} | ||
| this.fieldType && (optProps.type = this.fieldType) | ||
| this.fieldMessage && (optProps.message = this.fieldMessage) | ||
| return optProps |
There was a problem hiding this comment.
I wonder if it might be more straightforward to not have a default on these props and then just make fieldProps the values passed (which would be undefined if not set)? Or does the presence of those keys mess with something even if undefined?
| updateValue(key, val) { | ||
| this.value[key] = val | ||
| this.$emit('f-bidsmap', this.value) | ||
| }, |
There was a problem hiding this comment.
The combination of v-model and overwriting the @input event and then changing, but also emitting isn't a combination I've seen much (it kind of murkies a one-way data binding and a two-way). Are the v-models really needed? Could those maybe be just values? Or is there maybe an example/documentation that outlines this pattern that could be linked to in a comment?
| for (let i = 1; i < this.sessions.length; i++) { | ||
| key = this.sessions[i].participant_id | ||
| sval = this.sessions[i].s_series.join(' -s ') | ||
| for (let i = 1; i < this.xnat2bids.sessions.length; i++) { |
There was a problem hiding this comment.
this could probably be changed to for (const session of this.xnat2bids.sessions) and then could drop all the indexing (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of)
3 participants
This PR moves the Xnat2BidsForm to use the json schema. There are a couple of more complex sections that were tricky to get right with the basic form components via JSON so I packaged them as higher level components namely
Xnat2BidsSessionsandXnat2BidsBidsmap.I'm unsure yet what is the right balance between wrapping things into higher level components to keep the UI schema easier to make/read or to keep using low-level components only. For now, I try for a while and if it's too cumbersome I give up ;).
If one day I manage to make some of these inputs automatically update from other python packages, I'll need to revisit these higher level components as some of the "configurations" get embedded in them and are not exposed to the schema at the moment. But baby steps!
Also, I left the schema fairly simple. I'm planning to revisit it later when I try to figure out how to use the schema for validation more efficiently, because at the moment adding any more info to the schema (not the UI schema) doesn't really change anything as it is not getting used.