feat: DH-21375: Integrate databars with UI table format#1289
feat: DH-21375: Integrate databars with UI table format#1289
Conversation
|
@copilot Can you check how this PR compares to the plan at https://github.com/deephaven/deephaven-plugins/pull/1282/changes#diff-4b3243a5e436df978a8e62a555524a2fd6f899e8f65248f3879f74611bc0947d and comment if you think there are any discrepancies between the plan the this implementation? Do not open a new pull request, just let me know if there are any discrepancies |
|
@mattrunyon I've opened a new pull request, #1290, to work on those changes. Once the pull request is ready, I'll request review from you. |
gzh2003
requested review from
mattrunyon
and removed request for
margaretkennedy
gzh2003
force-pushed
the
DH-21375-databars-need-to-be-integrated-with-ui-table-format
branch
from
413b901 to
dbeccb8
Compare
plugins/ui/docs/components/table.md
Outdated
| > [!WARNING] | ||
| > Migrating from the `databars` parameter | ||
| > | ||
| > The legacy `databars` parameter is deprecated and will be removed in a future version. Use `format_` with `mode=ui.TableDatabar()` instead. |
There was a problem hiding this comment.
if gplus, maybe specific next version; if not, future version it is!
gzh2003
force-pushed
the
DH-21375-databars-need-to-be-integrated-with-ui-table-format
branch
from
abaf673 to
b8b6dad
Compare
|
ui docs preview (Available for 14 days) |
| k: convert_dataclasses_to_dicts(v) | ||
| for k, v in asdict(obj).items() | ||
| if v is not None |
There was a problem hiding this comment.
It looks like asdict is already recursive, so when you have a dataclass you can just return asdict(dataclass). The rest of the recursive might be required if the top level is an array or dict though as asdict only works on dataclasses.
https://docs.python.org/3/library/dataclasses.html#dataclasses.asdict
I tried this in a Python console
from dataclasses import dataclass
@dataclass
class Point:
x: int
y: int
@dataclass
class A:
val: dict
a = A({ 'abc': [Point(0, 0)], "xyz": Point(1, 1)})
asdict(a)
# {'val': {'abc': [{'x': 0, 'y': 0}], 'xyz': {'x': 1, 'y': 1}}}| if isinstance(date, DTypeInstant.j_type): # type: ignore | ||
| return _convert_instant_to_nanos(date) | ||
|
|
||
| if isinstance(date, DTypeZonedDateTime.j_type): | ||
| if isinstance(date, DTypeZonedDateTime.j_type): # type: ignore | ||
| tz = date.getZone() # type: ignore | ||
| instant = date.toInstant() # type: ignore | ||
| return (_convert_instant_to_nanos(instant), str(tz) if tz else None) | ||
|
|
||
| if isinstance(date, DTypeLocalDate.j_type): | ||
| if isinstance(date, DTypeLocalDate.j_type): # type: ignore |
There was a problem hiding this comment.
Why # type: ignore? Is there not a valid type for us to use?
There was a problem hiding this comment.
Pyright doesn't see the j_type attribute since in core we have cast(type, ...).
| return convert_dict_keys(dict, to_camel_case) | ||
|
|
||
|
|
||
| def convert_dataclasses_to_dicts(obj: Any) -> Any: |
There was a problem hiding this comment.
I'm not sure this is needed. We already do some recursive logic that converts dataclasses in arrays/dicts/props in Renderer.py
There was a problem hiding this comment.
Yeah, this is redundant. My main issue earlier was that None values in nested dicts/lists serialize as null.
I updated remove_empty_keys to handle this, so the Renderer path should take care of it now.
| t_databar_conditional = ui.table( | ||
| _stocks, | ||
| format_=[ | ||
| ui.TableFormat( | ||
| cols="Size", | ||
| if_="Size > 50", | ||
| mode=ui.TableDatabar(color="positive", max=1000), | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
The image for this doesn't seem to be correct
- It is rendering as a databar for all values of size
- It also is incorrectly overlapping the databar and the text . And I think the max isn't being applied properly? There is a 1090 value which loses its
1and then the values with3000have the text completely overlapped
The 2nd seems to be a problem on the current version as well though. I think there might be a problem in Grid handling this. Could you check if it's an issue with UITable or Grid and file a ticket? Basically any value over max is not getting capped at max when it should be
There was a problem hiding this comment.
For the second issue, it appears the bug is in the grid, as you mentioned. Filed a ticket here ticket.
Regarding:
It is rendering as a databar for all values of size
The conditional formatting appears to be working as expected. For example, in the screenshot, no databar is shown for the 7th or 12th row from the top. It’s possible I'm misunderstanding or the screenshot might be outdated.
gzh2003
force-pushed
the
DH-21375-databars-need-to-be-integrated-with-ui-table-format
branch
from
0975c05 to
3eaf0e7
Compare
|
ui docs preview (Available for 14 days) |
Part of DH-21375. Adds support for databars in
ui.TableFormatviamodeparameter. The legacydatabarsparameter isstill supported but will show a deprecation warningno longer supported.Changes:
There are some validation rules/warnings to be aware of while we maintain backwards compatibility with the old APIInitially, we considered keeping additional validation rules/warnings to preserve backward compatibility across multiple release cycles. After discussion, we decided to remove them and simplify the approach instead