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

fix: allows keys to be set in props #810

Merged
merged 29 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
72df595
fix: allows keys to be set in props
wusteven815 Sep 6, 2024
580a72e
fix: define generate_key for UITable
wusteven815 Sep 6, 2024
ceb0a54
fix: use Optional for 3.8 and 3.9
wusteven815 Sep 6, 2024
9062589
feat: experimental key prop decorator
wusteven815 Sep 6, 2024
44e7d6c
feat: experimental key prop decorator v2
wusteven815 Sep 6, 2024
1c17db0
feat: experimental element maker
wusteven815 Sep 9, 2024
9261c58
fix: use Dict for 3.8 and 3.9
wusteven815 Sep 9, 2024
3d2d7d1
fix: use Optional for 3.8 and 3.9
wusteven815 Sep 9, 2024
666052b
fix: use correct element name
wusteven815 Sep 10, 2024
e04fdd8
Merge branch 'deephaven:main' into 731-add-key-prop
wusteven815 Sep 10, 2024
5e90fed
feat: add docstring and key prop
wusteven815 Sep 11, 2024
97a4cb5
fix: use future annotations
wusteven815 Sep 11, 2024
979731d
feat: add all props
wusteven815 Sep 11, 2024
653c6af
fix: use title case instead of snake
wusteven815 Sep 11, 2024
8c139cc
Merge branch 'deephaven:main' into 731-add-key-prop
wusteven815 Sep 13, 2024
1890bfe
update prop description
wusteven815 Sep 17, 2024
0a0cf37
revert flex
wusteven815 Sep 17, 2024
fcf64d2
revert experiments
wusteven815 Sep 17, 2024
74ce8e2
use future annotation instead of optional
wusteven815 Sep 17, 2024
d840e10
add key to props
wusteven815 Sep 18, 2024
ff31aae
add key prop to components
wusteven815 Sep 18, 2024
a36ae23
add missing docs
wusteven815 Sep 18, 2024
17f363a
Merge branch 'deephaven:main' into 731-add-key-prop
wusteven815 Sep 18, 2024
b57ea8b
add key desc for fragment
wusteven815 Sep 18, 2024
ec3290b
remove kwargs
wusteven815 Sep 18, 2024
2516177
remove render map
wusteven815 Sep 18, 2024
2804408
remove generate_key method
wusteven815 Sep 19, 2024
08b8993
add e2e
wusteven815 Sep 19, 2024
b0e4602
Merge branch 'deephaven:main' into 731-add-key-prop
wusteven815 Sep 20, 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
33 changes: 33 additions & 0 deletions plugins/ui/src/deephaven/ui/components/flex.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations
from typing import Any
from inspect import signature, Parameter
from functools import wraps
from .basic import component_element
from .types import (
LayoutFlex,
Expand All @@ -12,6 +14,37 @@
)


def wrap_prop(f: Any, wrapper: Any, desc: str) -> Any:
sig = signature(f)
params = list(sig.parameters.values())
wrapper_params = list(signature(wrapper).parameters.values())[1:-1]

for wrapper_param in wrapper_params:
params.insert(1, wrapper_param)

wrapper = wraps(f)(wrapper)
wrapper.__signature__ = sig.replace(parameters=params)
wrapper.__doc__ += f" {desc}\n"

return wrapper


def add_key_prop(f: Any) -> Any:
def wrapper(*args, key: str | None = None, **kwargs):
return f(*args, key=key, **kwargs)

return wrap_prop(f, wrapper, "key: A unique key for the component.")


def add_qwerty_prop(f: Any) -> Any:
def wrapper(*args, qwerty: int | None = None, **kwargs):
return f(*args, qwerty=qwerty, **kwargs)

return wrap_prop(f, wrapper, "qwerty: qwerty.")


@add_qwerty_prop
@add_key_prop
wusteven815 marked this conversation as resolved.
Show resolved Hide resolved
def flex(
*children: Any,
flex: LayoutFlex | None = "auto",
Expand Down
7 changes: 3 additions & 4 deletions plugins/ui/src/deephaven/ui/components/make_component.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import functools
import logging
from typing import Any, Callable
from typing import Any, Callable, Optional
from .._internal import get_component_qualname
from ..elements import FunctionElement

Expand All @@ -17,9 +17,8 @@ def make_component(func: Callable[..., Any]):
"""

@functools.wraps(func)
def make_component_node(*args: Any, **kwargs: Any):
def make_component_node(*args: Any, key: Optional[str] = None, **kwargs: Any):
component_type = get_component_qualname(func)

return FunctionElement(component_type, lambda: func(*args, **kwargs))
return FunctionElement(component_type, lambda: func(*args, **kwargs), key=key)

return make_component_node
17 changes: 15 additions & 2 deletions plugins/ui/src/deephaven/ui/elements/BaseElement.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,35 @@ class BaseElement(Element):
Must provide a name for the element.
"""

def __init__(self, name: str, /, *children: Any, **props: Any):
def __init__(
self, name: str, /, *children: Any, key: str | None = None, **props: Any
):
self._name = name
self._keyProp = key

if len(children) > 0 and props.get("children") is not None:
raise ValueError("Cannot provide both children and props.children")

if len(children) > 1:
props["children"] = list(children)
if len(children) == 1:
# If there's only one child, we pass it as a single child, not a list
# There are many React elements that expect only a single child, and will fail if they get a list (even if it only has one element)
props["children"] = children[0]

self._props = dict_to_camel_case(props)

@property
def name(self) -> str:
return self._name

@property
def key_prop(self) -> str | None:
return self._keyProp

def generate_key(self, index_key: str) -> str:
if self._keyProp is not None:
return self._keyProp
return f"{index_key}-{self._name}"

def render(self, context: RenderContext) -> dict[str, Any]:
return self._props
20 changes: 20 additions & 0 deletions plugins/ui/src/deephaven/ui/elements/Element.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,26 @@ def name(self) -> str:
"""
return "deephaven.ui.Element"

@property
def key_prop(self) -> str | None:
"""
Get the key prop of this element. Useful to check if a key prop was provided.

Returns:
The unique key prop of this element.
"""
return None

@abstractmethod
def generate_key(self, index_key: str) -> str:
"""
Get the key of this element. Subclasses should access the key prop if it exists or generate their own unique key.

Returns:
The unique key of this element.
"""
pass

wusteven815 marked this conversation as resolved.
Show resolved Hide resolved
@abstractmethod
def render(self, context: RenderContext) -> PropsType:
"""
Expand Down
14 changes: 13 additions & 1 deletion plugins/ui/src/deephaven/ui/elements/FunctionElement.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@


class FunctionElement(Element):
def __init__(self, name: str, render: Callable[[], list[Element]]):
def __init__(
self, name: str, render: Callable[[], list[Element]], key: str | None = None
):
"""
Create an element that takes a function to render.

Expand All @@ -18,11 +20,21 @@ def __init__(self, name: str, render: Callable[[], list[Element]]):
"""
self._name = name
self._render = render
self._keyProp = key

@property
def name(self):
return self._name

@property
def key_prop(self) -> str | None:
return self._keyProp

def generate_key(self, index_key: str) -> str:
if self._keyProp is not None:
return self._keyProp
return f"{index_key}-{self._name}"

def render(self, context: RenderContext) -> PropsType:
"""
Render the component. Should only be called when actually rendering the component, e.g. exporting it to the client.
Expand Down
3 changes: 3 additions & 0 deletions plugins/ui/src/deephaven/ui/elements/UITable.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ def _with_dict_prop(self, key: str, value: dict[str, Any]) -> "UITable":
) # Turn missing or explicit None into empty dict
return UITable(**{**self._props, key: {**existing, **value}}) # type: ignore

def generate_key(self, index_key: str) -> str:
wusteven815 marked this conversation as resolved.
Show resolved Hide resolved
return f"{index_key}-{self.name}"

def render(self, context: RenderContext) -> dict[str, Any]:
logger.debug("Returning props %s", self._props)
return dict_to_camel_case({**self._props})
Expand Down
32 changes: 30 additions & 2 deletions plugins/ui/src/deephaven/ui/renderer/Renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ def _get_context_key(item: Any, index_key: str) -> Union[str, None]:
- TODO #731: use a `key` prop if it exists on the `Element`.
"""
if isinstance(item, Element):
return f"{index_key}-{item.name}"
if isinstance(item, (Dict, List, Tuple)):
return item.generate_key(index_key)
wusteven815 marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(item, (Dict, List, Tuple, map)):
return index_key
return None

Expand Down Expand Up @@ -61,6 +61,8 @@ def _render_item(item: Any, context: RenderContext) -> Any:
The rendered item.
"""
logger.debug("_render_item context is %s", context)
if isinstance(item, map):
return _render_map(item, context)
if isinstance(item, (list, tuple)):
wusteven815 marked this conversation as resolved.
Show resolved Hide resolved
# I couldn't figure out how to map a `list[Unknown]` to a `list[Any]`, or to accept a `list[Unknown]` as a parameter
return _render_list(item, context) # type: ignore
Expand All @@ -78,6 +80,32 @@ def _render_item(item: Any, context: RenderContext) -> Any:
return item


def _render_map(item: map, context: RenderContext) -> list[Any]:
"""
Renders a map object. Checks if each child has a unique key and then the map as a list

Args:
item: The map to render.
context: The context to render the map in.

Returns:
The rendered map.
"""
has_sent_error = False # Only throw the error once
logger.debug("_render_map %s", item)
with context.open():
result = []
used_keys = set()
for key, value in enumerate(item):
if isinstance(value, Element):
if value.key_prop in used_keys and not has_sent_error:
logger.error('Each child in a map should have a unique "key" prop.')
has_sent_error = True
used_keys.add(value.key_prop)
result.append(_render_child_item(value, str(key), context))
return result


wusteven815 marked this conversation as resolved.
Show resolved Hide resolved
def _render_list(
item: list[Any] | tuple[Any, ...], context: RenderContext
) -> list[Any]:
Expand Down
Loading