Slider does not reflect initial value in stream branch

[danyx23]

In the stream branch, the slider is all the way to the left (= 130 cm) even though the model value is 175 as is indicated by the text field (and a console.log). It should reflect the value of 175 cm.

[ivan-kleshnin]

I'm trying to help you. A few considerations before that:

1) Related issue https://github.com/staltz/cycle/issues/102 2) As intent, model, view etc are object producers, e.g. factories I think it's ok to capitalize them. It will improve readability, do you agree? 3) Custom elements are WC spoilers so should be capitalized. But CycleJS has some bug with that. I've just pinged Andre in gitter about that. 4) It's really weird that you use both Weight and Mass terms. Should be one of them.

[danyx23]

Thanks for your help!

1) Yes, if props could work like this that would be nice, would make it easier in the future to automate the injection too. 2) I have no strong opinion on this - from what I understand classical javascript reserves it for constructors and es6 uses it for classes, other than that it seems lowercase is prefered. But yes, they could be capitalized. 3) Yes in my original pre-stream draft is was capitalized, coming from React that seems right (and they are authoriative for jsx imho atm) 4) I guess you are right :). I just thought that in the ui mass is maybe a strange term as users probably think of the thing they read off the scale as their weight. Mass is what the original react+omniscient example uses. We could change the ui of course, since this is a sample it is probably confusing. So yes, let's change that. Should I do this or do you want to get started anyhow on it and send a PR?

[ivan-kleshnin]

I have no strong opinion on this - from what I understand classical javascript reserves it for constructors and es6 uses it for classes, other than that it seems lowercase is prefered. But yes, they could be capitalized.

There was such convention until React breaked that (for good or bad) and start to capitalize their own module name (being not factory, nor class). Now many people seems to prefer capitalized module names. Like Joi, Hapi etc.

---

For now I discovered only that the problem is with numbers, not with position in DOM. Values > 100 are influenced in a way you see with height.

Let's change that. Should I do this or do you want to get started anyhow on it and send a PR?

I replaced alot of code according to my own style preferences and continue to mess with it, so feel free to make it on yourself.

[danyx23]

Does it? In the second slider, if I change the initial value to 105 it goes to the correct position in the initial load.

[danyx23]

Oh no wait - it goes to what would be the position of 100

[danyx23]

Hm, can this be a html problem? As in: you have to set the range first, THEN update the value? If you do it in one go the behaviour is undefined/wrong? It's just weird that both FF and Chrome on win behave the same.

[ivan-kleshnin]

Yes, it looks like a bug. Sliders have 100 as a default value and this number is exactly the same I mentioned as an error threshold above. Very suspiciously.

[ivan-kleshnin]

All browsers behave the same. So not an issue with HTML implementation. I think the problem lies in that dom hack. Step parameter also does not work. That's a marker! Slider attributes are not passed correctly so the slider stays with the defaults and if value > 100 it's > max so the behavior is broken.

Let's try JSX transform instead of that:

// workaround for babel jsx -> hyperdom h. Babel currently does not create an array of children
// but simply creates more than 3 arguments, but vdom ignores this. This method fixes this
function dom(tag, attrs, ...children) {
  return h(tag, attrs, children);
}

[danyx23]

Yes it looks like it. I just converted the jsx to h() calls to see if the transformer drops something but the behaviour is the same. So I agree, it seems to be a vdom issue. I'll try to make a simple test case and open an issue with them

[ivan-kleshnin]

I've got working example :) Looks like adding step attribute "fixes" slider. Please backlink this issue if you will raise one on Virtual-DOM. I also want to track this bug or whatever it is.

/** @jsx dom */
let Cycle = require("cyclejs");
let {Rx, h, createStream: Stream, render, registerCustomElement} = Cycle;

// workaround for babel jsx -> hyperdom h. Babel currently does not create an array of children
// but simply creates more than 3 arguments, but vdom ignores this. This method fixes this
function dom(tag, attrs, ...children) {
  return h(tag, attrs, children);
}

// register custom slider element that displays a label, a slider and an editable numeric field
registerCustomElement("slider", (rootElement$, Props) => {
  let model = function() {
    let value$ = Stream((changeValue$, PropsStartValue$) =>
      PropsStartValue$.merge(changeValue$).shareReplay(1)
    );
    let min$ = Stream(Min$ => Min$.startWith(0).shareReplay(1));
    let max$ = Stream(Max$ => Max$.startWith(100).shareReplay(1));
    let step$ = Stream(Step$ => Step$.startWith(10).shareReplay(1));
    let label$ = Stream(Label$ => Label$.startWith("").shareReplay(1));
    return {
      value$, min$, max$, step$, label$,
      inject(Props, intent) {
        value$.inject(intent.changeValue$, Props.get("value$"));
        min$.inject(Props.get("min$"));
        max$.inject(Props.get("max$"));
        step$.inject(Props.get("step$"));
        label$.inject(Props.get("data-label$"));
        return intent;
    }};
  }();

  let view = function() {
    let vtree$ = Stream((value$, min$, max$, step$, label$) =>
      Rx.Observable.combineLatest(
        value$, min$, max$, step$, label$,
        function (value, min, max, step, label) {
          return (
            <div class="form-group">
              <label>{label}</label>
              <div className="input-group">
                <input className="form-control" type="range" value={value} min={min} max={max} step={step}/>
                <div className="input-group-addon">
                  <input type="text" value={value}/>
                </div>
              </div>
            </div>
          );
        })
    );
    return {
      vtree$,
      inject(model) {
        vtree$.inject(model.value$, model.min$, model.max$, model.step$, model.label$);
        return model;
      }
    };
  }();

  let user = function () {
    return {
      interactions$: rootElement$.interactions$,
      inject(view) {
        rootElement$.inject(view.vtree$);
        return view;
      }
    };
  }();

  let intent = (function () {
      let changeSlider$ = Stream(interactions$ =>
        interactions$.choose("[type=range]", "input")
          .map(event => parseInt(event.target.value, 10)));

        // here we want to filter invalid values so they don"t get pushed into the stream and the user can correct them.
        // alternatively we could make a stream of objects that have the parsed value or an error message, but since this
        // is a simple example, this will do.
      let changeInput$ = Stream(interactions$ =>
        interactions$.choose("[type=text]", "input")
          .map(event => parseInt(event.target.value, 10))
          .filter(val => !Number.isNaN(val)));
      return {
        changeSlider$,
        changeInput$,
        changeValue$: Rx.Observable.merge(changeSlider$, changeInput$),
        inject(user) {
          changeSlider$.inject(user.interactions$);
          changeInput$.inject(user.interactions$);
          return user;
        }
      };
  })();

    // wire up everything
  user.inject(view).inject(model).inject(Props, intent).inject(user);

    // expose only the changeValue$ stream to the outside.
    // tap is used to log changes in the value to the console.
  return {
    changeValue$: intent.changeValue$.tap(x => console.log("Slider changed to: " + x))
  };
});

let model = (function () {
  let height$ = Stream(changeHeight$ => changeHeight$.startWith(150));
  let weight$ = Stream(changeWeight$ => changeWeight$.startWith(150));
  return {
    height$,
    weight$,
    inject(intent) {
      height$.inject(intent.changeHeight$);
      weight$.inject(intent.changeWeight$);
      return intent;
    }
  }
})();

let view = (function () {
  let vtree$ = Stream((height$, weight$) => {
    return Rx.Observable.combineLatest(
      height$, weight$,
      function (height, weight) {
        return (
          <div>
            <div>
              <slider className="slider-height" label="Height (in cm):" value={height} min={0} max={200} step={20} key={1}/>
              <slider className="slider-weight" label="Weight (in kg): " value={weight} min={0} max={200} step={20} key={2}/>
            </div>
          </div>
        );
      });
  });
  return {
    vtree$,
    inject(model) {
      vtree$.inject(model.height$, model.weight$);
      return model;
    }
  }
})();

let user = (function () {
  let interactions$ = Stream(vtree$ => render(vtree$, ".app").interactions$);
  return {
    interactions$,
    inject(view) {
      interactions$.inject(view.vtree$);
      return view;
    }
  };
})();

let intent = (function() {
  let changeHeight$ = Stream(interactions$ => interactions$.choose(".slider-height", "changeValue").map(event => event.data));
  let changeWeight$ = Stream(interactions$ => interactions$.choose(".slider-weight", "changeValue").map(event => event.data));
  return {
    changeHeight$,
    changeWeight$,
    inject(user) {
      changeHeight$.inject(user.interactions$);
      changeWeight$.inject(user.interactions$);
      return user;
    }
  };
})();

// wire everything together
user.inject(view).inject(model).inject(intent).inject(user);

[danyx23]

Oh wow, thanks so much! I just diffed it and tried to find the minimal change that would make my version work - it turns out the startWith on the Label$ (!) does the trick. Change that and everything works. Will have to investigate how this is possible

[danyx23]

Ok, it actually makes sense. startsWith can be on any of the props, if it is on one of the min or max values than it has to be big enough to accomodate the value and maybe different from the default value. What either that or the startsWith on the label do is force a second roundtrip through the stream chain so that the vdom is updated once. Since this is not different from moving the slider or changing the value, it is kind of a workaround - it would be nice if a single roundtrip would suffice. Will try if I can replicate this problem with vdom alone.

[ivan-kleshnin]

To my experiments label does not help (because it's a separate element in HTML). You need to trigger <input/> update and that can be made with step or perhaps other input attribute. But again as long as value of slider < 100 it works as expected without anything additional. So there is something very strange here indeed.

[danyx23]

I opened an issue with virtual-dom about this problem

[danyx23]

It turns out the problem was the order of arguments. If min and max occur before value, everything works fine. I.e. this works:

<input className="form-control" type="range" min={min} max={max} value={value}/>

while this does not, if the initial value is outside the default min max params:

<input className="form-control" type="range" value={value} min={min} max={max}/>

Fixing the argument order as in the first example fixes this bug without the need for the startWith workaround.

[danyx23]

Reopening because order of keys in an object is undefined according to ECMA, this fix is a lucky coincidence that relies on an implementation detail of the js engine.