-
Notifications
You must be signed in to change notification settings - Fork 15
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: deephaven.ui plugin prototype initial checkin #47
Conversation
- Still need to clean up stuff
- Use pythons logging module
- So it doesn't conflict with anything running on 10000
- Create a hooks directory - Hooks now each have their own file
- Should make it cleaner when adding new components
- Whoops, forgot to delete old files
- HTML element exports attributes - Still need to wire it up to the render
- Add a bunch of functions for HTML elements - TODO: Now need to output as json and encode it
- JS side has not been refactored yet - Have a base Element type that can be used by HTML elements, other basic elements - FunctionElement type handles Elements that can re-render with a function - Separate components and object_types out into their own folder - Motivated by keeping separation of concerns. Theoretically the render/elements could all by in their own package that is not dependent on `deephaven` package at all, which may be advantageous to separate out at some point (same rendering backend with different rendering frontend) - Would need to move the use_table_listener hook as well
- Builds the array of exported objects
- Traverses through the tree and renders it - Have not done HTML elements yet
- Now can wrap stuff in the html elements.
- Will need to add some test cases for components in general
- Will need to wire this up with Spectrum anyway - Following example works: ``` import deephaven.ui as ui from deephaven.ui import html, use_state @ui.component def stock_widget_table(source, default_sym="", default_exchange=""): sym, set_sym = use_state(default_sym) exchange, set_exchange = use_state(default_exchange) ti1 = ui.text_field(sym, on_change=set_sym) ti2 = ui.text_field(exchange, on_change=set_exchange) t1 = ( source.where([f"sym=`{sym.upper()}`", f"exchange=`{exchange.upper()}`"]) if sym and exchange else html.h1("Please enter Sym and Exchange Above") ) return [ui.flex_row(ti1, ti2), t1] import deephaven.plot.express as dx stocks = dx.data.stocks() swt = stock_widget_table(stocks, "", "") ```
- No callbacks from HTML yet
- Looking good
- Now should be able to do callbacks generically based on props
- Instead of re-using t, use a different variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only reviewed JS code
? new JSONRPCServerAndClient( | ||
new JSONRPCServer(), | ||
new JSONRPCClient(request => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit what jsonrpc brings to the table for us? Just curious as at a glance it looks like it's mostly just to wrap our existing widget rpc calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it provides an existing standard for sending messages/notifications between client/server. The grpc layer does not require a response per request, but with json-rpc there is a spec for a message that requires a response which is useful for callbacks.
fetch(): Promise<JsWidget>; | ||
} | ||
|
||
function ElementPanel(props: ElementPanelProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's still early, but jsdocs for the components and their purpose would be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I'll include that in the next PR that moves a bunch of this logic - it was just in ElementPanel to use the convenient useDashboardPanel
hook, but we'll need to put a lot of this logic in to the DashboardPlugin
level so that we can open multiple panels.
const debouncedOnChange = useDebouncedCallback( | ||
propOnChange, | ||
VALUE_CHANGE_DEBOUNCE | ||
); | ||
|
||
const onChange = useCallback( | ||
newValue => { | ||
setValue(newValue); | ||
debouncedOnChange(newValue); | ||
}, | ||
[debouncedOnChange] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might make sense as a separate hook if we start using this over and over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this and also event handlers will probably want some sort of hook or wrapper that we can re-use.
- Renamed `spectrum_wrappers` to just `spectrum` - Externalize logging - Requires a change in web-client-ui as well - Other cleanups
- Add typings to RenderContext - Add comments in a couple places - Fix up NodeEncoder so that it optimizes how items are added
- Code is already in the examples/README file
- There's a bunch of flex stuff that still needs to be cleaned up/figured out in the general layout of things.
plugins/ui/examples
folder should workImplemented so far:
@ui.component
decorator (Fixes Create @ui.component #54)FunctionElement
object when called.FunctionElement
is then "rendered" (e.g. function is run) when theFunctionElement
object is exported (seeFunctionElementMessageStream
) (Fixes Create render context/lifecycle #52)hooks
folder for the hooks available (use_state
,use_memo
,use_effect
, etc)text_field
andtext
types are implemented