Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify value behavior #1154

Closed
mturoci opened this issue Dec 14, 2021 · 4 comments · Fixed by #2118
Closed

Unify value behavior #1154

mturoci opened this issue Dec 14, 2021 · 4 comments · Fixed by #2118
Assignees
Labels
feature Feature request ui Related to UI

Comments

@mturoci
Copy link
Collaborator

mturoci commented Dec 14, 2021

via @psinger

Repro

from h2o_wave import main, app, Q, ui


@app("/demo")
async def serve(q: Q):
    if not q.client.initialized:
        items = [
            ui.slider(name="test", label="test", value=0.5, min=0, max=1, step=0.1),
            ui.dropdown(name="test2", label="test2", value="1", choices=[ui.choice("1", "1"), ui.choice("2", "2"), ui.choice("3", "3")]),
            ui.button(name='submit', label='Submit', primary=True),
        ]
        q.page['example'] = ui.form_card(box='1 1 4 4', items=items)
        q.client.initialized = True
    else:
        items=[
            ui.slider(name="test", label="test", value=0.3, min=0, max=1, step=0.1),
            ui.dropdown(name="test2", label="test3", value="2", choices=[ui.choice("1", "1"), ui.choice("2", "2"), ui.choice("3", "3")]),
            ui.button(name='show_inputs', label='Submit', primary=True),
        ]
        q.page['example'].items = items
    await q.page.save()

Actual behavior

After changing dropdown value and hitting submit, dropdown value should respect user input, but is currently overwritten by the value attribute in the else block (2).

Desired behavior

value should be initial value only and dropdown should then be fully controlled by the user.

@mturoci mturoci added ui Related to UI bug Bug in code labels Dec 14, 2021
@aalencar aalencar self-assigned this Dec 15, 2021
@aalencar
Copy link
Contributor

I couldn't find a solution yet, but I found out why dropdown doesn't hold its value when submit is clicked.

@mturoci gave me some leads. form card and bond function might have something to do with it. After much examination, I didn't find anything there. I removed bond , turned form into a class component, and tinkered with componentDidUpdate, but the problem persisted.

Instead of checking why the dropdown was changing, I decided to investigate why the Slider component was not. Turns out that it was, but Fluent Slider's internal logic handles updates differently, not changing its value back to the default when props change. Also, contrarily to what was previously assumed, components are not being recreated when submit is clicked.

Based on that, I had to look into how Fluent's Dropdown logic works and the snippet bellow can help us understand what is going on.

  // fluentui/packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.base.tsx

  public UNSAFE_componentWillReceiveProps(newProps: IDropdownProps): void {
    // In controlled component usage where selectedKey is provided, update the selectedIndex
    // state if the key or options change.
    let selectedKeyProp: 'defaultSelectedKeys' | 'selectedKeys' | 'defaultSelectedKey' | 'selectedKey';

    // this does a shallow compare (assumes options are pure), for the purposes of determining whether
    // defaultSelectedKey/defaultSelectedKeys are respected.
    const didOptionsChange = newProps.options !== this.props.options;

    if (newProps.multiSelect) {
      if (didOptionsChange && newProps.defaultSelectedKeys !== undefined) {
        selectedKeyProp = 'defaultSelectedKeys';
      } else {
        selectedKeyProp = 'selectedKeys';
      }
    } else {
      if (didOptionsChange && newProps.defaultSelectedKey !== undefined) {
        selectedKeyProp = 'defaultSelectedKey';
      } else {
        selectedKeyProp = 'selectedKey';
      }
    }

    if (
      newProps[selectedKeyProp] !== undefined &&
      (newProps[selectedKeyProp] !== this.props[selectedKeyProp] || didOptionsChange)
    ) {
      this.setState({
        selectedIndices: this._getSelectedIndexes(newProps.options, newProps[selectedKeyProp]),
      });
    }

    if (
      newProps.options !== this.props.options // preexisting code assumes purity of the options...
    ) {
      this._sizePosCache.updateOptions(newProps.options);
    }
  }

Pay attention to const didOptionsChange = newProps.options !== this.props.options. options in this case refers to the ui.dropdown choices arguments in our Wave app.

this.props.options: initial choices argument
newProps.options: choices argument provided when submit is clicked

Using @mturoci's repro code I notice that the didOptionChanges is always true. The main issue is the shallow comparison.

Even though the choices arguments in the Wave app have the same values, they are created in different places, holding different references, so when we compare them with !== (reference inequality) the result is true.

As you can see in the if inside the outermost else block, when didOptionChanges is true, selectedKeyProp is set to 'defaultSelectedKey' which will be used to fetch the value for the input box in the following if block.

I don't know how the references are being kept for objects and arrays between the Wave app and React, but I imagine it's a good place to explore possible root causes. @lo5 and @mturoci maybe can give me a hand with this one.

@aalencar
Copy link
Contributor

aalencar commented Jan 4, 2022

I'm currently looking into other components to check if they have the same issue. If so, we'd have to come up with a more generic approach that solves the problem on a higher level in the component tree.

@aalencar aalencar linked a pull request Jan 10, 2022 that will close this issue
aalencar added a commit that referenced this issue Jan 14, 2022
@mturoci mturoci changed the title Some form elements are still recreated instead of updated Unify value behavior Mar 31, 2022
@mturoci mturoci added feature Feature request and removed bug Bug in code labels Mar 31, 2022
@aalencar
Copy link
Contributor

aalencar commented Sep 20, 2022

Supports setting value from Wave App:

⚠️ Behavior for setting choice-based components must be unified as well.

Screen.Recording.2022-09-20.at.9.29.56.mov

We also have value for some stat cards but the value that is more likely to be updated is data and we already have many examples for that, proving that they work.

Repro code

To be tested against master.

from h2o_wave import main, app, Q, ui

choices = [
    ui.choice("A", "Choice A"),
    ui.choice("B", "Choice B"),
    ui.choice("C", "Choice C"),
]

tabs = [
    ui.tab(name="email", label="Mail", icon="Mail"),
    ui.tab(name="events", label="Events", icon="Calendar"),
    ui.tab(name="spam", label="Spam"),
]


@app("/demo")
async def serve(q: Q):

    if q.args.reset:
        q.client.initialized = False

    if not q.client.initialized:
        q.page["controls"] = ui.form_card(box="1 1 2 1", items=[
            ui.buttons(items=[
                ui.button(name="change", label="Change", primary=True),
                ui.button(name="reset", label="reset", primary=True), ]
            )
        ])

        q.page["tab"] = ui.tab_card(
            box="1 2 4 1",
            items=[ui.tab(name="#menu/a", label="A"), ui.tab(name="#menu/b", label="B"), ui.tab(name="#menu/c", label="C"), ui.tab(name="#borken", label="(broken)")],
            value="#menu/b",
        )

        q.page["nav"] = ui.nav_card(
            box="1 3 2 -1",
            value="#menu/b",
            items=[
                ui.nav_group("Menu (working)", items=[
                    ui.nav_item(name="#menu/a", label="A"),
                    ui.nav_item(name="#menu/b", label="B"),
                    ui.nav_item(name="#menu/c", label="C"),
                ])
            ],
        )

        q.page["broken"] = ui.form_card(box="3 1 4 -1", items=[
            ui.text_xl(name="broken", content="BROKEN"),
            ui.checkbox(name="checkbox", label="Checkbox", value=False),
            ui.date_picker(name="date", label="Date Picker", value="2017-10-19"),
            ui.toggle(name="toggle", label="Toggle"),
            ui.spinbox(name="spinbox", label="Spinbox", min=0, max=100, step=10, value=30),
            ui.range_slider(name="range_slider_step", label="Range Slider", min=0, max=1000, step=100, min_value=0, max_value=300),
            ui.slider(name="slider", label="Slider", min=0, max=100, step=10, value=30),
            ui.time_picker(name="timepicker_disabled", label="Time Picker", value="11:15"),
            ui.copyable_text(label="Copyable text", value="foo"),
            ui.choice_group(name="choice_group", label="Choice Group  (can set choices, does not clear selection)", value="B", choices=choices),
            ui.picker(name="picker", label="Picker (can set choices, does not clear selection)", choices=choices, values=["A"]),
        ])

        q.page["working"] = ui.form_card(box="7 1 4 -1", items=[
            ui.text_xl(name="broken", content="WORKING"),
            ui.progress(label="Progress", value=0.5),
            ui.tabs(name="menu", value="spam", items=tabs),
            ui.textbox(name="textbox", label="TextBox", value="foo"),
            ui.combobox(name="combobox", label="Combobox single (can set choices, clears selection)", value="a", choices=["a", "b", "c"]),
            ui.combobox(name="combobox2", label="Combobox multi (can set choices, clears selection)", values=["a"], choices=["a", "b"]),
            ui.checklist(name="checklist", values=["A", "B"], label="Checklist (can set choices, does not clear selection)", choices=choices),
            ui.color_picker(name="color", label="Color picker (can set choices, does not clear selection)", value="yellow", choices=["yellow", "green"])
        ])

        q.client.initialized = True

    if q.args.change:
        q.page["broken"].items[1].checkbox.value = True
        q.page["broken"].items[2].date_picker.value = "2022-03-22"
        q.page["broken"].items[3].toggle.value = True
        q.page["broken"].items[4].spinbox.value = 50
        q.page["broken"].items[5].range_slider.min_value = 50
        q.page["broken"].items[5].range_slider.max_value = 60
        q.page["broken"].items[6].slider.value = 50
        q.page["broken"].items[7].time_picker.value = "13:00"
        q.page["broken"].items[8].copyable_text.value = "bar"
        q.page["broken"].items[9].choice_group.value = "D"
        q.page["broken"].items[9].choice_group.choices = [ui.choice("B", "Choice B"), ui.choice("C", "Choice C"), ui.choice("D", "Choice D")] # can be set but does not clear the selection
        q.page["broken"].items[10].picker.values = ["B"]
        q.page["broken"].items[10].picker.choices = [ui.choice("B", "B")] # can be set but does not clear the selection
        q.page["tab"].value = "#menu/c"

        q.page["working"].items[1].progress.value = 0.75
        q.page["working"].items[2].tabs.value = "events"
        q.page["working"].items[3].textbox.value = "changed"
        q.page["nav"].value = "#menu/c"
        # To test dropdown use https://github.com/h2oai/wave/pull/1621#issue-1372662766
        q.page["working"].items[4].combobox.value = "b"
        q.page["working"].items[5].combobox.choices = ["b", "c"] # can be set and clears the selection
        q.page["working"].items[5].combobox.values = ["b"]
        q.page["working"].items[5].combobox.choices = ["b", "c"] # can be set and clears the selection
        q.page["working"].items[6].checklist.values = ["B"]
        q.page["working"].items[6].checklist.choices = [ui.choice("B", "Choice B")]
        q.page["working"].items[7].color_picker.value = "#FFFFF8"
        q.page["working"].items[7].color_picker.choices = ["yellow", "green", "orange"]  # can be set but does not clear the selection

    await q.page.save()

@mturoci
Copy link
Collaborator Author

mturoci commented Jun 20, 2023

@marek-mihok can you please finish this once you are back from vacation? You can either make a single PR or multiple smaller ones, whatever suits you.

mturoci added a commit that referenced this issue Aug 15, 2023
mturoci pushed a commit that referenced this issue Aug 24, 2023
marek-mihok added a commit that referenced this issue Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request ui Related to UI
Projects
None yet
3 participants