Make mv-options work with expressions, fixes #777 #972
Conversation
src/primitive.js
Outdated
| @@ -1316,6 +1316,9 @@ Mavo.observe({id: "primitive"}, function({node, type, attribute, record, element | |||
| else if (attribute === "mv-options") { | |||
| node.updateOptions(); | |||
|
|
|||
| let option = node.options.get(node.value) ?? node.options.keys().next().value; | |||
There was a problem hiding this comment.
Or should we take into account the default value? 🤔
let option = node.options.get(node.value) ?? node.default ?? node.options.keys().next().value;
There was a problem hiding this comment.
The default value will be taken into account inside the node anyway, so this is not the right place to deal with it, and it would be an undersirable coupling (now every time we touch default values, we also need to look in the code that deals with mv-options updates).
In fact, I wonder if even this line might be duplicating logic that should be elsewhere. Isn't there already logic that sets the value appropriately based on node.options? If so, we should be invoking that…
There was a problem hiding this comment.
@LeaVerou, I could manage to find a place where, from my perspective, this logic should be. Could you please give this PR another shot?
| if (!this.options.has(value)) { | ||
| // There is no option corresponding to the value. Try the default one. | ||
| // If there is no option corresponding to the default value either, use the first option | ||
| value = this.options.has(this.default)? this.default : this.options.keys().next().value; |
There was a problem hiding this comment.
We should not change the value if it doesn't match any of the options, as that's destructive. The user may be experimenting with a different UI, or have forgotten to add all pertinent options. As a design principle, loading data into any Mavo and then saving it back should be a non-destructive operation.
For <select> there is a whole code path that creates (and removes when no longer needed) .mv-volatile options in the <select> to deal with this. mv-options should behave similarly.
There was a problem hiding this comment.
That makes perfect sense. Thanks. I assume I must think about it a bit more.
There was a problem hiding this comment.
FWIW there is already a code path around a property's editor mutating, you should probably hook into that, since that's what a changing mv-options is, conceptually.
There was a problem hiding this comment.
Possibly not for v0.3.0 then?
I was going to suggest this. I might have been a bit too optimistic about the issue. 😅
I thought it would be nice if such an awesome feature like
mv-optionswould work with expressions correctly in the upcoming release. :)