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

feat: add undefined type option #1026

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

wusteven815
Copy link
Contributor

@wusteven815 wusteven815 commented Nov 18, 2024

  • Closes Python - Differentiate null from undefined #549
  • Add a new Undefined object for nullable props
    • Nullable props can be declared with _nullable_props passed to component_element/BaseElement
    • For these props, None will be translated to null and Undefined will not exist in the props object

@wusteven815 wusteven815 self-assigned this Nov 18, 2024
@wusteven815
Copy link
Contributor Author

wusteven815 commented Nov 18, 2024

Example:

from deephaven import ui


@ui.component
def nullish_calendar():
    return ui.flex(
        ui.calendar(value=ui.types.Undefined),
        ui.calendar(value=None),
    )


calendar_test = nullish_calendar()
  • For Undefined, the calendar can still be edited, while for None, the calendar can not be edited

@wusteven815 wusteven815 marked this pull request as ready for review November 18, 2024 23:09
plugins/ui/src/deephaven/ui/_internal/utils.py Outdated Show resolved Hide resolved
plugins/ui/src/deephaven/ui/_internal/utils.py Outdated Show resolved Hide resolved
plugins/ui/src/deephaven/ui/types/types.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Code changes look good to me. I like how this approach simplifies things. Left 1 comment to update comment block with new _nullable_props arg. Also would be good to put something about this in the UI docs.

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

@wusteven815 as @bmingles suggested let's add a couple docs for this as well.

  • An example to picker.md showing the use of None vs Undefined
  • A subsection in architecture.md components, named "Props" where we detail how we're mapping React JS props to Python args in general (children are mapped to positional args, other props are mapped to keyword args, along with a couple small examples), then a subsection within that for "Handling null vs. undefined" and detail how the issue (JS has null/undefined, some components require that differentiation, Python only has None), and how we're addressing it.

Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Suggested changes for controlled vs uncontrolled description / examples.

Co-authored-by: Brian Ingles <[email protected]>
plugins/ui/docs/components/picker.md Outdated Show resolved Hide resolved
@wusteven815 wusteven815 dismissed stale reviews from mofojed and bmingles December 9, 2024 18:54

Added

mofojed
mofojed previously approved these changes Dec 10, 2024
bmingles
bmingles previously approved these changes Dec 10, 2024
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM

plugins/ui/docs/architecture.md Outdated Show resolved Hide resolved
plugins/ui/docs/architecture.md Outdated Show resolved Hide resolved
plugins/ui/docs/components/picker.md Outdated Show resolved Hide resolved
@wusteven815 wusteven815 dismissed stale reviews from bmingles and mofojed via c789ba2 December 12, 2024 16:25
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.

Python - Differentiate null from undefined
5 participants