Conversation
chrisknoll
left a comment
There was a problem hiding this comment.
When I bring up the preview, and change some of the existing elements in the preview, but then click 'cancel', the existing elements that I changed remain changed.
Should the preview use a 'copy' of the concept set expression for preview, and then 'apply' copies the copy into the final concept set expression?
|
@chrisknoll Please note the |
| self.isExcluded = ko.observable(data.isExcluded && data.isExcluded() || false); | ||
| self.includeDescendants = ko.observable(data.includeDescendants && data.includeDescendants() || false); | ||
| self.includeMapped = ko.observable(data.includeMapped && data.includeMapped() || false); |
There was a problem hiding this comment.
Don't do this, data should never come in as an observable to the constructor.
If you want to make a 'copy' and re-observe it, take the origional, do a ko.toJS() on the object, then re-instantiate.
ie:
var origional; // this is the existing thing to clone
var clone = new ConceptSetItem(ko.toJS(origional))
There was a problem hiding this comment.
Ok, reverted
| const existingConceptsCopy = this.conceptSetStore.current() | ||
| ? this.conceptSetStore.current().expression.items().map(item => new ConceptSetItem(item)) | ||
| : []; | ||
| this.previewConcepts(itemsToAdd.concat(existingConceptsCopy)); |
There was a problem hiding this comment.
Here, you would just say:
const existingConceptsCopy = new ConceptSet(ko.toJS(this.conceptSetStore.current()));
Although what i'm suggesting above assumed that the ConceptSetPreview would be working off of a ConceptSet and not an array of ConceptSetItems...which is fine but in that case, I should say that the change is:
const existingConceptsCopy = this.conceptSetStore.current()
? this.conceptSetStore.current().expression.items().map(item => new ConceptSetItem(ko.toJS(item)))
: [];
And then, when the 'Apply' button is clicked in the preview editor, that will overwrite this.conceptSetStore.current() with the new state of the ConceptSetItems.
But, overall, you have the right approach: you need to clone the existing concept set for the user to modify, and only 'commit' the change when they click 'Apply' and discard those changes (ie: do nothing) if they cancel.
There was a problem hiding this comment.
Note, that last part (where when the click is made, the finalized change is saved back to the current concept set...if we're working off a clone, doesn't the changes made to the clone need to propagate tack to the object?
There was a problem hiding this comment.
Fixed.
Cloned concepts are concatenated with selected in the search results. They all stored in this.previewConcepts() and passed to current concept set in the addPreviewConcepts() method.
|
The My concern here is that the preview behavior seems to be standard across any context (show a modal with the concept set editor with the clone) so could this behavior be embedded inside the ConceptAddBox component? If it makes sense to have the component hosting the ConceptAddBox component provide their own impelementation of 'onPreview' then what you have works, but I was just curious if there will be a need to have context-specific implementations. Your implementation does provide that flexiblity, but it does mean we need to make sure that search, included concept, source concept tabs all provide a handler for onPreview param. |
…button if handler is not specified
|
@chrisknoll Also I fixed multiple CS stores behavior - please review. |
|
@anton-abushkevich from discussion today on the Atlas WG, we feel that having such a concept set preview would be useful wherever we work with Concept Sets in the user interface. So if we could modify this PR to have this capability, that would be a great addition to this enhancement. |
Resolves #2791