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.

Sign up for GitHub

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

[ENG-4153]Use empty string for None values in rx.input and rx.el.input #4521

Merged
merged 10 commits into from
Dec 12, 2024
Merged

[ENG-4153]Use empty string for None values in rx.input and rx.el.input #4521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3de01d1
Use empty string for None values in `rx.input` and `rx.el.input`
ElijahAhianyo Dec 11, 2024
302b810
fix tests
ElijahAhianyo Dec 11, 2024
6b20287
fix pyi scripts
ElijahAhianyo Dec 11, 2024
76034a1
use nullish coalescing operator
ElijahAhianyo Dec 11, 2024
768e15a
fix unit tests
ElijahAhianyo Dec 11, 2024
1cddb88
use ternary operator so old browsers and dynamic components tests pass
ElijahAhianyo Dec 11, 2024
31462ad
address comment on doing this only for optionals
ElijahAhianyo Dec 12, 2024
437d421
Fix tests
ElijahAhianyo Dec 12, 2024
fb8e28c
pyright!
ElijahAhianyo Dec 12, 2024
7286ec5
fix comments
ElijahAhianyo Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions reflex/components/el/elements/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
prevent_default,
)
from reflex.utils.imports import ImportDict
from reflex.utils.types import is_optional
from reflex.vars import VarData
from reflex.vars.base import LiteralVar, Var

Expand Down Expand Up @@ -384,6 +385,33 @@ class Input(BaseHTML):
# Fired when a key is released
on_key_up: EventHandler[key_event]

@classmethod
def create(cls, *children, **props):
"""Create an Input component.

Args:
*children: The children of the component.
**props: The properties of the component.

Returns:
The component.
"""
from reflex.vars.number import ternary_operation

value = props.get("value")

# React expects an empty string(instead of null) for controlled inputs.
if value is not None and is_optional(
(value_var := Var.create(value))._var_type
):
props["value"] = ternary_operation(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like adding a ternary to every value prop on every text input is a bit excessive. maybe we can check isoptional(value._var_type) and apply the ternary in that case.

This does potentially create an issue where the Var is not typed optional, but still ends up with a None value, however the fix for that is to improve the annotation of the Var.

I know it's probably negligible and maybe not worth optimizing for, but perhaps we should profile text field re-render time with and without a ternary in the value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like adding a ternary to every value prop on every text input is a bit excessive. maybe we can check isoptional(value._var_type) and apply the ternary in that case.

This would be great and keep the generated code simpler and faster for values which will never be None

This does potentially create an issue where the Var is not typed optional, but still ends up with a None value, however the fix for that is to improve the annotation of the Var.

I agree, we should not bother too much about wrongly annotated vars.

I know it's probably negligible and maybe not worth optimizing for, but perhaps we should profile text field re-render time with and without a ternary in the value?

While profiling is a great idea, I think it's a bit overkill if we only do this for optional values. However, I do not want to stop anyone from doing that :D

(value_var != Var.create(None)) # pyright: ignore [reportGeneralTypeIssues]
& (value_var != Var(_js_expr="undefined")),
value,
Var.create(""),
)
return super().create(*children, **props)


class Label(BaseHTML):
"""Display the label element."""
Expand Down
4 changes: 2 additions & 2 deletions reflex/components/el/elements/forms.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ class Input(BaseHTML):
on_unmount: Optional[EventType[[], BASE_STATE]] = None,
**props,
) -> "Input":
"""Create the component.
"""Create an Input component.

Args:
*children: The children of the component.
Expand Down Expand Up @@ -576,7 +576,7 @@ class Input(BaseHTML):
class_name: The class name for the component.
autofocus: Whether the component should take the focus once the page is loaded
custom_attrs: custom attribute
**props: The props of the component.
**props: The properties of the component.

Returns:
The component.
Expand Down
15 changes: 15 additions & 0 deletions reflex/components/radix/themes/components/text_field.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
from reflex.components.core.debounce import DebounceInput
from reflex.components.el import elements
from reflex.event import EventHandler, input_event, key_event
from reflex.utils.types import is_optional
from reflex.vars.base import Var
from reflex.vars.number import ternary_operation

from ..base import LiteralAccentColor, LiteralRadius, RadixThemesComponent

Expand Down Expand Up @@ -96,6 +98,19 @@ def create(cls, *children, **props) -> Component:
Returns:
The component.
"""
value = props.get("value")

# React expects an empty string(instead of null) for controlled inputs.
if value is not None and is_optional(
(value_var := Var.create(value))._var_type
):
props["value"] = ternary_operation(
(value_var != Var.create(None)) # pyright: ignore [reportGeneralTypeIssues]
& (value_var != Var(_js_expr="undefined")),
value,
Var.create(""),
)

component = super().create(*children, **props)
if props.get("value") is not None and props.get("on_change") is not None:
# create a debounced input if the user requests full control to avoid typing jank
Expand Down
Loading