Seeking community consensus on how updates to input expressions should function #1642

Closed
rsbondi opened this Issue Mar 4, 2016 · 41 comments

Projects

None yet
@rsbondi
Contributor
rsbondi commented Mar 4, 2016

I am posting this to get feedback on how input expressions on update should be handled. See further discussion at #1612 & #1638

<input value={expression} name="myInput" onchange={changed}>
this.expression = "initial value"

The riot functionality has been to evaluate {expression} with "initial value". On additional calls to update, {expression} will only be evaluated if the value of this.expression has changed. The user can type freely into the input field but riot will not evaluate the expression on update as long as the value for this.expression has not changed. A change of this.expression will then overwrite any user input on update. Additionally, you can manually sync the two like

changed() {
  this.expression = myInput.value
}

Interest has been shown suggesting that a different approach may be desired. That would be to force a synchronized state by always evaluating the expression on update. So the user types in values, but the expression will be evaluated on update returning the input field to "initial value" or whatever the current value is for this.expression The dom updates would be limited by comparing the dom value to this.expression. The above manual sync would be required to preserve the input value on update. See more here

Please help us decide if we should keep the existing functionality in the 3.0 release or implement the alternate method

@ilblog
ilblog commented Mar 4, 2016

Well RIOT is nice because it is tiny and fast. As @GianlucaGuarini said, it is very close to native JS. Making easier life for developers, means more code and less speed. Current RIOT solution seems to be right for me personally. This is what I use.

    <div if={ renaming }>
        <input type="text" onblur={ blur } onkeyup={ changeName } value={ fav.name }>
    </div>  

    this.blur = function() {
        self.renaming = false;
        // Save new name to DB;
    }

    this.changeName = function(e) {

        if(e.keyCode === 13)  // Enter
            self.blur()
        else 
            newName = e.target.value; 
    }

Guys, RIOT is not Angular JS or some big fat framework. Think of RIOT JS lake Rapsbery Pi. Small tiny toy for sophisticated programmer.

@dogada
dogada commented Mar 4, 2016

IMO, Riot should NEVER discard user input. Input value should be treated as
initial only and after DOM-node for the input is constructed it should
ignore any expression updates.

When I need to change an input value I should do it implicitly
this.myInputName.value = newValue.

Regards,
Dima.

On Fri, Mar 4, 2016 at 3:52 AM, Richard Bondi notifications@github.com
wrote:

I am posting this to get feedback on how input expressions on update
should be handled. See further discussion at #1612
#1612 & #1638
#1638

this.expression = "initial value"

The riot functionality has been to evaluate {expression} with "initial
value". On additional calls to update, {expression} will only be
evaluated if the value of this.expression has changed. The user can type
freely into the input field but riot will not evaluate the expression on
update as long as the value for this.expression has not changed. A change
of this.expression will then overwrite any user input on update.
Additionally, you can manually sync the two like

changed() {
this.expression = myInput.value
}

Interest has been shown suggesting that a different approach may be
desired. That would be to force a synchronized state by always evaluating
the expression on update. So the user types in values, but the expression
will be evaluated on update returning the input field to "initial value"
or whatever the current value is for this.expression The dom updates
would be limited by comparing the dom value to this.expression. The above
manual sync would be required to preserve the input value on update. See
more here
http://blog.iansinnott.com/managing-state-and-controlled-form-fields-with-react/

Please help us decide if we should keep the existing functionality in the
3.0 release or implement the alternate method


Reply to this email directly or view it on GitHub
#1642.

@sylvainpolletvillard
Contributor

What you are describing by forcing the state looks like an <output>, not an <input>. I don't see any good reason to prevent user input by default on an input tag.

@gregorypratt
Contributor

Is this what you're talking about?

http://plnkr.co/edit/uIqFsVKfCef1lHdKsRAa?p=preview

If you make changes, the template reverts to the initial value on blur.

@GianlucaGuarini
Member

@gregorypratt exactly, the next riot@2.3.17 will fix this issue but we don't know if this is a good behavior to introduce in riot@3.0.0 so we want to know what you think guys

@gregorypratt
Contributor

From the example at the top:

<input value={expression} name="myInput" onchange={changed}>
this.expression = "initial value"

this bit is important: value={expression} this tells Riot to apply the value of expression to the input.

That's all you need to know really. And unless you actually change expression why would you expect the input to change?

If you were to use this.myInput.value everywhere however you wouldn't have an issue. It would always be in-sync because you're in direct control of the template. You're not asking Riot to do anything automatically for you.

This behaviour is what I'd expect.

@aMarCruz
Member
aMarCruz commented Mar 4, 2016

We are going to nowhere.
I think we can set a flag in riot.options to enable the new behavior, for those guys coming from React or for special requirements.

BTW:
people used to working with data-driven apps (like me) usually use "disconnected" fields in edit mode. Allow user edition in forms linked directly to the database or data store is risky (crazy?).
That's why I see the change proposed by @GianlucaGuarini as something natural; the current behavior has always confused me.

@yuretz
yuretz commented Mar 4, 2016

@rsbondi wrote

The riot functionality has been to evaluate {expression} with "initial value". On additional calls to update, {expression} will only be evaluated if the value of this.expression has changed. The user can type freely into the input field but riot will not evaluate the expression on update as long as the value for this.expression has not changed.

Well, is it still true for inputs inside a loop without no-reorder tag? If not much has changed regarding loop rendering since the version I use (2.3.1), then any user edits are discarded whenever looped input triggers an event, because the whole loop is re-rendered on any looped child update.

My personal vote goes for consistency. As for me, I can live with any solution, as long as the behavior/logic is the same in all the situations (also in loops!).

Having a flag in riot.options is of course a possibility, but that means increasing the complexity, which is just a way to more bugs and inconsistencies. So I would prefer if you could choose one true riot way (hopefully, the simplest one, with the least "magic" involved) and stick to it, even though some users might disagree.

@yuretz
yuretz commented Mar 4, 2016

Thinking a little bit more about the topic in question, the issue #1319 might also be relevant for this discussion, because besides value, things like input focus and cursor position are another part of an input state, and it looks like it gives some weird results if we allow riot resetting those on each update.

@rsbondi
Contributor
rsbondi commented Mar 5, 2016

@yuretz I think the issue is "with" no-reorder, it is fine without but point taken about consistency.

@jmk2142
jmk2142 commented Mar 5, 2016

After some mild initial whiplash between the prior behavior and newer behavior, I think the most consistent behavior with what I understand as the "riot way" is the original. this.data is the source of truth and unless that's updated (explicitly) the expression shouldn't be re-computed and updated. Having an onchange listener to explicitly set state isn't a difficult extra step and makes the intent very clear. A short blurb in the docs about how input values work, to reemphasize this one directional state driven behavior would hopefully solve any confusion.

I suppose there are ways that both behaviors could be supported, like options or perhaps a special attribute to indicate that the particular input will be synced with the source data but part of the beauty of riot is that it doesn't have a lot of special directives and such.

@mattjcowan

2.3.17 seems to be working again like 2.3.15 was, in these regards, nice! The 2.3.17 implementation works nicely for me.The problem with 2.3.16 is very apparent when using onkeyup events without updating the expression.
I'd agree with @jmk2142, and also close enough to React imo ...
It would be nice to see some documentation/example on editing rows for example in a grid context (inline-editing), and discussing in further detail when it would be good to update an existing entity vs a copy of the entity, and how to "reset" fields to their original values, for newer folks, more of a design pattern. Just a thought, as I'm trying to help others with the framework, and the general concepts, and figure out best patterns myself.
Btw, the plunker from @gregorypratt is now pointing to 2.3.17, so got me confused at first, just a thought that using script tags to specific versions of Riot (ie: cloudflare) can help understand the intent of the plunker better over time, for late-comers (6 days, lol) like me :-) ...

@gregorypratt
Contributor

Looking at the plunker (that is now using 2.3.17 as pointed out by @mattjcowan) I think I prefer this behaviour...

@whatda
whatda commented Mar 11, 2016

I'm sorry for the confusion I may have caused with #1612. As I've noted in a later comment ( #1612 (comment) ) the previous riot behavior actually made much more sense and a special "refresh"-method could be a better option to implement this behavior. Thanks at @rsbondi for explaining it to me.

@aMarCruz
Member

An additional option in update should be enough, very useful to restore the data in a form.

update([data,] true) --> true forces an inconditional update of the UI.

EDIT: I mean, updates the UI if it value is different from the expression.

@jmk2142
jmk2142 commented Mar 12, 2016

@whatda No need to apologize. Issues are how discussions are raised that bring about consensus as to where to go right? I've delved deeper into this and in the end, came out understanding Riot even better. :-)

This was referenced Mar 19, 2016
@GianlucaGuarini
Member

Ok here I would like to let our user voting democratically what's the best behavior for riot in this case, I would like to avoid new api methods, flags or whatsoever attributes. I will make 2 posts here and the one receiving more 👍 will be the way to go

@GianlucaGuarini
Member
GianlucaGuarini commented Mar 19, 2016 edited

Please vote here for a version where riot must always update the input/select tags giving priority to the data coming from the update events discarding in this case the user input events
example

@GianlucaGuarini
Member

Please vote here for a version where riot should update the input/select tags only when the original value passed initially to them changes independently from the user input
example

@aMarCruz aMarCruz referenced this issue Mar 20, 2016
Closed

Selects in 2.3.17 reset after onchange event #1667

1 of 7 tasks complete
@ilearnio

I think that updating of the input tag while the user is typing, is a really bad idea. Especially if it would be textarea where he might enter a lot of text and then suddenly lose all the changes. So my vote is for the second variant

This was referenced Mar 20, 2016
@willemmulder

So how does this work now? If we update the original input value, the input field will be updated? If so, how would we handle the case where the original input value remains the same, but we still want to discard user input?

@GianlucaGuarini
Member
GianlucaGuarini commented May 6, 2016 edited

@willemmulder The next riot major release will update the input values only if the original value ( of the model/object/store bound to it ) will be changed according to our community feedback #1642 (comment)

@willemmulder

@GianlucaGuarini Yes thanks, I saw that. So for me, the question remains how we can reset an input field to its bound value, discarding user populated values, without actually changing the bound value. Or should I simply do a quick "backup bound value Y", then "change bound value to X", then "change bound value back to Y" to simulate a change in the bound value?

@GianlucaGuarini
Member
GianlucaGuarini commented May 6, 2016 edited

@willemmulder oh sorry I assumed it was clear http://plnkr.co/edit/9qrn6Mb6zzpIj9WawGSU?p=preview
Of course you don't handle all this logic directly in a tag, but you do it somewhere else using redux/flux/whathever script

@GianlucaGuarini
Member
GianlucaGuarini commented May 6, 2016 edited

@willemmulder I have tried to emulate a kind of simpler redux logic here http://plnkr.co/edit/VRkTQdrctcd8Vn1ilxfj?p=preview
EDIT:
I have also added all the possible features you can handle in the form, reset included

@willemmulder

@GianlucaGuarini Ah that makes sense! On update, Riot compares the bound values to see if they have changed, and so we need to wrap our values in a String() object for example, so the pointer-comparison will be false. The easiest that I could come up with is this, which seems to work: http://plnkr.co/edit/Va0d4DMWLRNEJpbOIXIM?p=preview. Is my thinking correct? Thanks again!

@GianlucaGuarini
Member

@willemmulder yes your demo looks fine to me you got it ;)

@willemmulder

@GianlucaGuarini Thanks! :-)

@bradleypeabody

Has anyone looked at the idea of adding a "formRefresh" (or other appropriate name) method to a Tag which would just explicitly cause all value={} expressions to copy from the source data newly? From looking at the thread above and contemplating some different use cases, it seems to me that when/if you want to force this kind of update is dependent upon the use case, and if you do want it, there is usually a pretty clear place to put that call - you just don't want to have to list out a bunch of this.input1.value = ''. this.input2.value = ''; etc. and keep that up to date as your form evolves. Having a simple call that just explicitly says "yes, update my form values from the underlying data, dammit!" would seem to be very useful.

@nodefish
nodefish commented Jun 16, 2016 edited

I feel like Vue.js has a good solution for this, by supporting an initial bind feature, written as {{* expression }}

(i.e. asterisk at the start of the mustache)

A common use case is calling a handler on the oninput event in order to generate an autocomplete / suggestions list while the user is typing. In riot's current behavior, using oninput causes the user input to be lost, as handlers trigger updates (and if you use e.preventUpdate=true, then the autocomplete list won't be updated).

@aMarCruz
Member

update needs a flag, as commented here.

@brauliobo
brauliobo commented Aug 5, 2016 edited

both proposals (#1642 (comment) and #1642 (comment)) don't handle the updates made by the user on the input to sync values with the component variables. how this can or will be done? (as @bradleypeabody described)

@whatda
whatda commented Aug 27, 2016

Is there any workaround for a form refresh right now? Because I really need to solve this issue in my app. :S

Example: I have a modal for both editing and creating entities. After editing an entity, saving it and then choosing to create a new one, the input values are still filled with the values of the edit...

I think an update flag would be the best idea, because this is apparently not just some trivial "nice to have"-feature, but a serious restraint when working with forms.

@willemmulder
willemmulder commented Aug 27, 2016 edited

@whatda yes we have the exact same problem. An attribute like < form refresh={isFresh}> would be perfect. Then if we set isFresh=true and subsequently call update(), all the containing input-fields would be refreshed from their bind-expressions.

@seanzer
seanzer commented Sep 7, 2016

It should be up to the developer to update value. There is a placeholder attribute that should be used for placeholders. Value should always be updated.

@rsbondi rsbondi closed this Oct 24, 2016
@willemmulder

@rsbondi I see you closed the issue, but I'm still unclear to what I should do to update a form-field to use it's current bind-expression and get its new value from there... My mistake probably, but what is the suggested way of doing it, right now?

@GianlucaGuarini
Member

@willemmulder the way you (users) have decided #1642 (comment)

@willemmulder
willemmulder commented Oct 24, 2016 edited

@GianlucaGuarini Okay, so if we change the value on the model, the form-field will refresh if we call .update(). That's fine. But if we want to to force a refresh (without changing the model), we'd have to go over the model and change all the properties to e.g. model.value = new String(model.value); so that pointer comparison reports a change, even though the value is the same.

There is no escape from doing that, right?

@GianlucaGuarini
Member

we'd have to go over the model and change all the properties to e.g. model.value = new String(model.value); so that pointer comparison reports a change, even though the value is the same.

You can also sync your data simply listening the onchange event http://plnkr.co/edit/aDEaHlykV4iXtXPEIa9M?p=preview . If you will use a mixin for it it would be everything easier

@willemmulder

@GianlucaGuarini Yes good point. That's what I usually do.

The case where I needed a 'hard reset' of the form to its input-expressions is after some strange behaviour where if I switch the model of a field, somehow the old values of the field in the form are preserved. But I should actually try to find and fix that issue instead of working around it. If I find it, I'll post back here.

Thanks!

@whatda
whatda commented Nov 8, 2016

@GianlucaGuarini I don't intend to be rude here, but imo the survey was a bit flawed: There was no option for an extra update-flag or method, just update behavior in general. Given that, it's no wonder people voted for the old style, because a flag or extra method wasn't up for debate (maybe it would be better if it would have been up for debate).

@GianlucaGuarini GianlucaGuarini referenced this issue Dec 19, 2016
Closed

textbox value does not change #2166

2 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment