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

[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

Conversation

ElijahAhianyo
Copy link
Collaborator

React expects empty strings instead of null values for the value prop of the input component. putting this in a cond allows the input field to be updated when the value is None

Copy link

linear bot commented Dec 11, 2024

@benedikt-bartscher
Copy link
Contributor

Closes #4445

Thanks, @ElijahAhianyo!

I would love to have an integration test for the use case mentioned in my issue. I can add it if you want


# React expects an empty string(instead of null) for uncontrolled inputs.
if value is not None:
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

@ElijahAhianyo
Copy link
Collaborator Author

ElijahAhianyo commented Dec 12, 2024

Closes #4445

Thanks, @ElijahAhianyo!

I would love to have an integration test for the use case mentioned in my issue. I can add it if you want

yeah sure, feel free to add to this :),and ill just focus on the other comments

@ElijahAhianyo
Copy link
Collaborator Author

For now, the code below should work:

import reflex as rx
import random


class State(rx.State):
    name: rx.Field[str | None] = "Hello world"


def index() -> rx.Component:
    return rx.container(
        rx.button("Clear", on_click=lambda: State.set_name(None)),
        rx.text("rx.input"),
        rx.input(value=State.name, on_change=State.set_name),
        rx.text("rx.el.input"),
        rx.el.input(value=State.name),
    )


app = rx.App()
app.add_page(index)

However, we'll need this #4528 to work without rx.Field:

class State(rx.State):
    name: str | None = "Hello world"
  # name: Optional[str] = "Hello world"
    

masenf
masenf previously approved these changes Dec 12, 2024

value = props.get("value")

# React expects an empty string(instead of null) for uncontrolled inputs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: aren't these controlled inputs when a value is passed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah yeah, good catch

@masenf masenf merged commit 7ca50c6 into main Dec 12, 2024
41 checks passed
@masenf masenf deleted the elijah/input-field-none-field branch December 12, 2024 23:24
@benedikt-bartscher
Copy link
Contributor

Integration test is here: #4448

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants