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
fix: make "value" behavior consistent #1171
Conversation
@mturoci |
Thanks for the investigation @aalencar. I just checked and my previous statements about components still being recreated instead of updated are not true, not sure what led me there. I would like to avoid having to do a deep comparison if possible (especially because JSON.parse/stringify are blocking). The problem seems to be that Fluent is not sure what choices should be checked so that it looks for hints in |
We could freeze
Would do you mean exactly? Using only |
This is a no-go, we want to let people update choices.
https://reactjs.org/docs/uncontrolled-components.html Yes, using selectedKey(s) together with local value state. |
Agreed.
Ok, I will try it out. |
I just checked other controlled components and looks like there are other two that might have the same issue: ✅ checkbox from h2o_wave import main, app, Q, ui
@app("/demo")
async def serve(q: Q):
if not q.client.initialized:
items = [
ui.checklist(name='checklist', label='Checklist', values=["Spam"], choices=[
ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']
]),
ui.color_picker(name='color_picker', value='#000000', label='Color picker'),
ui.button(name='submit', label='Submit', primary=True),
]
q.page['example'] = ui.form_card(box='1 1 4 10', items=items)
q.client.initialized = True
else:
items = [
ui.checklist(name='checklist', label='Checklist', values=["Bacon"], choices=[
ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']
]),
ui.color_picker(name='color_picker', value="#ffffff", label='Color picker 2'),
ui.button(name='submit', label='Submit', primary=True),
]
q.page['example'].items = items
await q.page.save() |
@mturoci how do you think a controlled component would prevent the current behavior? |
I put up a minimal fix example (needs to be refined ofc), but it seem to work against repro code in the issue. I marked added lines with comments. Please have a look for general approach. BaseDropdown = ({ name, label, required, disabled, value, values, choices, trigger, placeholder }: Dropdown) => {
const
isMultivalued = !!values,
[singleSelection, setSingleSelection] = React.useState<S | U | undefined>(value), // Make this component controlled.
selection = React.useMemo(() => isMultivalued ? new Set<S>(values) : null, [isMultivalued, values]),
[selectedOptions, setSelectedOptions] = React.useState(Array.from(selection || [])),
options = (choices || []).map(({ name, label, disabled }): Fluent.IDropdownOption => ({ key: name, text: label || name, disabled })),
onChange = (_e?: React.FormEvent<HTMLElement>, option?: Fluent.IDropdownOption) => {
if (option) {
const optionKey = option.key as S
if (isMultivalued && selection !== null) {
option.selected ? selection.add(optionKey) : selection.delete(optionKey)
const selectedOpts = Array.from(selection)
wave.args[name] = selectedOpts
setSelectedOptions(selectedOpts)
} else {
setSingleSelection(option.key) // Update local state whenever the user selects a value.
wave.args[name] = optionKey
}
}
if (trigger) wave.push()
},
selectAll = () => {
if (!selection) return
selection.clear()
options.forEach(o => { if (!o.disabled) selection.add(o.key as S) })
const selectionArr = Array.from(selection)
setSelectedOptions(selectionArr)
wave.args[name] = selectionArr
onChange()
},
deselectAll = () => {
if (!selection) return
selection.clear()
setSelectedOptions([])
wave.args[name] = []
onChange()
}
return (
<>
<Fluent.Dropdown
data-test={name}
label={label}
placeholder={placeholder}
options={options}
required={required}
disabled={disabled}
multiSelect={isMultivalued || undefined}
selectedKeys={isMultivalued ? selectedOptions : undefined} // Tell Fluent what values to use - multiValued.
selectedKey={singleSelection} // Tell Fluent what value to use.
onChange={onChange}
/>
{
isMultivalued &&
<div>
<Fluent.Text variant='small'>
<Fluent.Link onClick={selectAll}>Select All</Fluent.Link> | <Fluent.Link onClick={deselectAll}>Deselect All</Fluent.Link>
</Fluent.Text>
</div>
}
</>
)
} |
1e63bbe
to
6f8415f
Compare
@mturoci, I just pushed a commit with your changes. Tested manually for single select and multi select, and it works as expected. 👍 Regarding refinements, I didn't spot anything that could be improved since not many changes were introduced. Just by looking at the GIF, could you confirm if these two components are broken as well? |
You are right, these have the same behavior as dropdown. The important point of this issue is to unify the In order to avoid more confusion than necessary, I think we should implement the desired behavior once for all rather than fix these 3 first and then rewrite them anyway. Another thing to think about is that current dropdown behavior works only by relying on choices having different references between component rerenders. Can it cause trouble in future? Is there any better way how to achieve the same? |
Taking a look at this one. |
6f8415f
to
995cab2
Compare
Wave app to test changes: from h2o_wave import main, app, Q, ui
choices = [
ui.choice('A', 'Option A'),
ui.choice('B', 'Option B'),
ui.choice('C', 'Option C', disabled=True),
ui.choice('D', 'Option D'),
]
combobox_choices = ['Cyan', 'Magenta', 'Yellow', 'Black']
@app("/demo")
async def serve(q: Q):
if not q.client.initialized:
items = [
ui.checkbox(name='checkbox', label='checkbox1', value=True),
ui.choice_group(name='choice_group', label='Pick one', value='A', required=True, choices=choices),
ui.color_picker(name='color', label='Pick a color', value='#F25F5C'),
ui.combobox(name='combobox', label='Enter or choose a color', placeholder='Color...', value='Blue',
choices=combobox_choices),
ui.date_picker(name='date', label='Standard date picker', value='2017-10-19'),
ui.textbox(name='textbox_disabled', label='Disabled', value='textbox 1'),
ui.progress(label='Standard Progress', caption='Downloading the interwebs...', value=0.25),
ui.slider(name='slider', label='Standard slider', min=0, max=100, step=10, value=30),
ui.copyable_text(label='Copyable text', value='hello!'),
ui.toggle(name='toggle', value=True),
ui.button(name='submit', label='Submit', primary=True),
]
q.page['example'] = ui.form_card(box='1 1 4 12', items=items)
q.client.initialized = True
else:
items = [
ui.checkbox(name='checkbox', label='checkbox2', value=False),
ui.choice_group(name='choice_group', label='Pick one', value='B', required=True, choices=choices),
ui.color_picker(name='color', label='Pick a color', value='#FFFFFF'),
ui.combobox(name='combobox', label='Enter or choose a color', placeholder='Color...', value='Yellow',
choices=combobox_choices),
ui.date_picker(name='date', label='Standard date picker', value='2022-11-20'),
ui.textbox(name='textbox_disabled', label='Disabled', value='textbox 2'),
ui.progress(label='Standard Progress', caption='Downloading the interwebs...', value=0.5),
ui.slider(name='slider', label='Standard slider', min=0, max=100, step=10, value=50),
ui.copyable_text(label='Copyable text', value='world!'),
ui.toggle(name='toggle', value=False),
ui.button(name='show_inputs', label='Submit', primary=True),
]
q.page['example'].items = items
await q.page.save() |
Looks like I forgot to change the Nav component, working on it and checking whether other components need it too. |
@aalencar This could be helpful when identifying all the widgets that needs to go through this ^^: |
|
I'm having a problem setting the selected item for Nav component on user click. From XNav = (model: State & { hideNav?: () => void }) => {
const { items, hideNav } = model
const [value, setValue] = useControlledComponent(model)
const groups = items.map((g): Fluent.INavLinkGroup => ({
name: g.label,
collapseByDefault: g.collapsed,
links: g.items.map(({ name, label, icon, disabled, tooltip }): Fluent.INavLink => ({
key: name,
name: label,
icon,
disabled,
title: tooltip,
style: disabled ? { opacity: 0.7 } : undefined,
url: '',
onClick: () => {
setValue(name)
if (hideNav) hideNav()
if (name.startsWith('#')) {
window.location.hash = name.substr(1) // Line causing the problem
return
}
wave.args[name] = true
wave.push()
}
}))
}))
return <Fluent.Nav groups={groups} selectedKey={value} />
|
ui/src/date_picker.tsx
Outdated
@@ -85,7 +85,7 @@ export const | |||
<Fluent.DatePicker | |||
data-test={m.name} | |||
label={m.label} | |||
value={value} | |||
value={transform(value)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mturoci, I chose to use transform before passing it down to Fluent's component, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is what I was talking about on our last call. Just give it a better non-generic name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted transform
from some code that we had before but looks like it's doing the same job as parseDate
.
Why is this a problem? Rerenders are fine, that's what your local state hook is for, or ...? |
The problem:
I wonder if I'm right about 4., this is a React matter. |
I see, so what are the alternatives we can solve this? |
It's still not clear to me why the props are changing and I couldn't reproduce the problem consistently. Sometimes the first click doesn't work, sometimes it does. Let me work on it again and I should have something by the end of the day. |
I could not find a solution for Nav but I can confirm that it's being recreated: @mturoci do you know why components are recreated when @mtanco, regarding this message on slack, do you know which components were impacted? @lo5 can you say if these changes to "value" behavior break old apps? |
@aalencar I believe the |
I could not repro this. What does your python code look like? |
@aalencar a few more things that need improvement. Updating value is not always working (didn't check for other components) @app('/demo')
async def serve(q: Q):
if not q.client.initialized:
q.page['example'] = ui.form_card(box='1 1 4 10', items=[
ui.checkbox(name='checkbox_unchecked', label='Checkbox'),
ui.button(name='btn', label='Change to true'),
])
q.client.initialized = True
if q.args.btn:
q.page['example'].items[0].checkbox.value = True
await q.page.save() Go through @app('/demo')
async def serve(q: Q):
if not q.client.initialized:
q.page['example'] = ui.form_card(box='1 1 4 10', items=[
ui.dropdown(name='dropdown', label='Choices', values=[],
choices=[ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']]),
ui.button(name='change', label='Change', primary=True),
])
q.client.initialized = True
if q.args.change:
q.page['example'].items[0].dropdown.values = ['Spam', 'Bacon']
await q.page.save() |
I just pushed a commit (WIP) with changes on Nav to use the custom hook and
Actually I didn't try to update the values directly like this: I believe updating the value directly is the most intuitive way and might be what most people will do. I will check why it's not working. Thanks for taking a look :) |
I just realized that there's a big flaw with updating |
Yes, the problem is your python code. You create a new card on every single render (serve) cycle - that's why your component gets (rightfully) recreated. Instead, you should create the nav card only once and update its value only. Something very roughly like this # Nav
# Use nav cards to display #sidebar #navigation.
# ---
from h2o_wave import main, app, Q, ui
persona = 'https://images.pexels.com/photos/220453/pexels-photo-220453.jpeg?auto=compress&h=750&w=1260'
@app('/demo')
async def serve(q: Q):
if not q.client.initialized:
q.client.initialized = True
q.page['nav1'] = ui.nav_card(
box='1 1 2 -1',
value='#menu/spam',
title='H2O Wave',
subtitle='And now for something completely different!',
image='https://wave.h2o.ai/img/h2o-logo.svg',
items=[
ui.nav_group('Menu', items=[
ui.nav_item(name='#menu/spam', label='Spam'),
ui.nav_item(name='#menu/ham', label='Ham'),
ui.nav_item(name='#menu/eggs', label='Eggs', tooltip='Make me a scrambled egg.'),
ui.nav_item(name='#menu/toast', label='Toast', disabled=True),
]),
ui.nav_group('Help', items=[
ui.nav_item(name='#about', label='About', icon='Info'),
ui.nav_item(name='#support', label='Support', icon='Help'),
])
],
secondary_items=[
ui.inline(items=[
ui.persona(title='John Doe', subtitle='Software developer', size='s', image=persona),
ui.menu(items=[
ui.command(name='profile', label='Profile', icon='Contact'),
ui.command(name='preferences', label='Preferences', icon='Settings'),
ui.command(name='logout', label='Logout', icon='SignOut'),
])
]),
],
)
q.page['form'] = ui.form_card(box='1 3 2 2', items=[
ui.button(name='change', label='Change'),
])
if q.args.change:
q.page['nav1'].value = 'your_val'
await q.page.save() |
656c47c
to
978d7ad
Compare
978d7ad
to
b17a197
Compare
@mturoci please take a look again at my solution. Nav and TabsProblem:
Solution: Destructuring PropsI could not understand yet why some components work with destructed parameters like Checklist XChecklist = ({ model: m }: { model: Checklist }) => {
...
[choices, setChoices] = useControlledComponent(m, getMappedChoices()), while others like checkbox don't and I have to pass XCheckbox = (props: { model: Checkbox }) => {
...
[value, setValue] = useControlledComponent(props, props.model.value) I didn't manage to repro on stackblitz I Found Another BugWe have to listen for every prop change in order to set the value to the one passed by the author. The problem is that if the user sets another parameter such as Why we don't listen only to from h2o_wave import main, app, Q, ui
@app('/demo')
async def serve(q: Q):
if not q.client.initialized:
q.page['example'] = ui.form_card(box='3 4 4 7', items=[
ui.checkbox(name='checkbox', label='Checkbox', value=False),
ui.button(name='change', label='Change', primary=True),
])
q.client.initialized = True
if q.args.change:
q.page['example'].items[0].checkbox.value = True
await q.page.save() Repro:
Test all changesfrom h2o_wave import main, app, Q, ui
persona = 'https://images.pexels.com/photos/220453/pexels-photo-220453.jpeg?auto=compress&h=750&w=1260'
choices = [
ui.choice('A', 'Option A'),
ui.choice('B', 'Option B'),
ui.choice('C', 'Option C', disabled=True),
ui.choice('D', 'Option D'),
]
combobox_choices = ['Cyan', 'Magenta', 'Yellow', 'Black']
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):
content = 'Welcome to our store!'
location = q.args['#']
if location:
if location == 'menu/spam':
content = "Sorry, we're out of spam!"
elif location == 'menu/ham':
content = "Sorry, we're out of ham!"
elif location == 'menu/eggs':
content = "Sorry, we're out of eggs!"
elif location == 'about':
content = 'Everything here is gluten-free!'
if not q.client.initialized:
q.page['example'] = ui.form_card(box='3 4 4 10', items=[
ui.checkbox(name='checkbox', label='Checkbox', value=False),
ui.dropdown(name='dropdown', label='Choices', value='',
choices=[ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']]),
ui.dropdown(name='dropdown2', label='Choices', values=[],
choices=[ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']]),
ui.dropdown(name='dropdown3', label='Choices', popup='always', value='',
choices=[ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']]),
ui.dropdown(name='dropdown4', label='Choices', popup='always', values=[],
choices=[ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']]),
ui.checklist(name='checklist', values=['Egg', 'Spam'], label='Choices',
choices=[ui.choice(name=x, label=x) for x in ['Egg', 'Bacon', 'Spam']]),
ui.choice_group(name='choice_group', label='Pick one', value='B', required=True, choices=choices),
ui.color_picker(name='color', label='Pick a color', value='#F25F5C'),
ui.combobox(name='combobox', label='Enter or choose a color', placeholder='Color...', value='Blue',
choices=combobox_choices),
ui.copyable_text(label='Copyable text', value='foo'),
ui.date_picker(name='date', label='Standard date picker', value='2017-10-19'),
ui.picker(name='picker', label='Place an order (try Spam, Eggs or Ham):', choices=[
ui.choice(name='spam', label='Spam'),
ui.choice(name='eggs', label='Eggs'),
ui.choice(name='ham', label='Ham'),
ui.choice(name='cheese', label='Cheese'),
ui.choice(name='beans', label='Beans'),
ui.choice(name='toast', label='Toast'),
], values=['eggs']),
ui.progress(label='Indeterminate Progress', caption='Goes on forever', value=0.5),
ui.slider(name='slider', label='Standard slider', min=0, max=100, step=10, value=30),
ui.spinbox(name='spinbox', label='Standard spinbox', min=0, max=100, step=10, value=30),
ui.tabs(name='menu', value='spam', items=tabs),
ui.toggle(name='toggle', label='Not checked'),
ui.button(name='change', label='Change', primary=True),
])
q.page['tab'] = ui.tab_card(
box='3 1 4 1',
items=[
ui.tab(name='#menu/spam', label='Spam'),
ui.tab(name='#menu/ham', label='Ham'),
ui.tab(name='#menu/eggs', label='Eggs'),
ui.tab(name='#about', label='About'),
],
value='#menu/ham',
)
q.page['nav'] = ui.nav_card(
box='1 1 2 -1',
value='#menu/spam',
title='H2O Wave',
subtitle='And now for something completely different!',
items=[
ui.nav_group('Menu', items=[
ui.nav_item(name='#menu/spam', label='Spam'),
ui.nav_item(name='#menu/ham', label='Ham'),
ui.nav_item(name='#menu/eggs', label='Eggs', tooltip='Make me a scrambled egg.'),
ui.nav_item(name='#menu/toast', label='Toast'),
])
],
)
q.page['blurb'] = ui.markdown_card(
box='3 2 4 2',
title='Store',
content=content,
)
q.client.initialized = True
if q.args.change:
q.page['example'].items[0].checkbox.value = True
q.page['example'].items[1].dropdown.value = 'Bacon'
q.page['example'].items[2].dropdown.values = ['Spam', 'Bacon']
q.page['example'].items[3].dropdown.value = 'Bacon'
q.page['example'].items[4].dropdown.values = ['Spam', 'Bacon']
q.page['example'].items[5].checklist.values = ['Bacon']
q.page['example'].items[6].choice_group.value = 'A'
q.page['example'].items[7].color_picker.value = '#FFFFF8'
q.page['example'].items[8].combobox.value = 'Yellow'
q.page['example'].items[9].copyable_text.value = 'bar'
q.page['example'].items[10].date_picker.value = '2022-03-22'
q.page['example'].items[11].picker.values = ['ham', 'toast']
q.page['example'].items[12].progress.value = 0.75
q.page['example'].items[13].slider.value = 50
q.page['example'].items[14].spinbox.value = 50
q.page['example'].items[15].tabs.value = 'events'
q.page['example'].items[16].toggle.value = True
q.page['tab'].value = '#menu/spam'
q.page['nav'].value = '#menu/spam'
if location:
blurb = q.page['blurb']
blurb.content = content
await q.page.save() |
when hash changes, this handler is invoked that sends all the changes to the server, causing a new rerender. I suggest putting breakpoints at both python and javascript side so that it's clear to you what is going on.
Not sure what "work" means here exactly.
|
You can have a look at 3f820b3.
I see, you are saying that the problem is the state is not reset between rerenders / serves. So what are the alternatives/possible solutions here? |
Ignoring changes to props other than
|
Task
Prevent dropdown from setting the value back to its initial value on re-renders.
Problem
In our
Dropdown
component, we were creating a newoptions
array on every re-renderSolution
Create a new
options
array when the new providedchoices
props contains different objects (deeply compare it usingJSON.stringify
)