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

docs: Component Rules #1055

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

Conversation

dgodinez-dh
Copy link
Contributor

@dgodinez-dh dgodinez-dh commented Dec 4, 2024

  • DOC-204


## Component Return Values

You can return three values from a `deephaven.ui` component: a component, a list of components, or `None`. Returning a single component will render that component at the root level of a panel. Returning a list of components will render all the components in a panel. Returning `None` should be used when a component is used to perform logic but does not need to be rendered.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mofojed this isn't true? For instance you can also return hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not know you could return hooks from a component. Can you also return primitive values like strings, numbers, and booleans?

Copy link
Member

Choose a reason for hiding this comment

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

@dsmmcken What do you mean return hooks? You can return the result of a hook, but returning the hook itself doesn't make sense (ditto in React). The result of the hook could be a component, list of components, None, or a primitive.
You can also return primitives though, such as str, int, boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed an update. I'm still not sure if this is saying what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I tested dict and set. They do not open up a panel.
With bool, it opens a panel, but does not render anything (where string and int get rendered).

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's correct. I don't know why you'd want to return a boolean (since it doesn't get rendered), but React does allow you to do that.
Returning a dict or a set could possibly give you an error. There's a little playground at the bottom of the React page where you can play with what are valid returns, we should match that: https://react.dev/learn/your-first-component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to add a code playground in our docs? Can we do that in an md file?

Copy link
Member

Choose a reason for hiding this comment

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

We want to do that, it will not be in the first deployment.
With React Playground it's nice because it all runs client side. We need to have a server running for our playground, so setup is a little more complicated. Probably use a running instance that's shared between all viewers of the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean return hooks? You can return the result of a hook,

Yeah, that's what I meant, like return [my_state, my_set_state]

Copy link
Member

Choose a reason for hiding this comment

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

@dsmmcken doing return [my_state, my_set_state] does not make sense from a component. You can do that from other hooks, but that doesn't make sense in React or in deephaven.ui.
You can do return my_state. You wouldn't return the setter function.

@dgodinez-dh dgodinez-dh requested a review from mofojed December 4, 2024 21:52
mofojed
mofojed previously approved these changes Dec 6, 2024
my_component("Hello World", prop1="value1")
```

## Defining Your Own Children and Props
Copy link
Contributor

Choose a reason for hiding this comment

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

sentence case for headings, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@margaretkennedy margaretkennedy left a comment

Choose a reason for hiding this comment

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

Looks good to me after the typo fix is committed.

margaretkennedy
margaretkennedy previously approved these changes Dec 9, 2024
Copy link
Contributor

@jjbrosnan jjbrosnan left a comment

Choose a reason for hiding this comment

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

Only one comment for now, since my next questions are what are at the forefront of my mind as I read this.

What's the reason this and other PRs are not in the docs repo? Do we really expect users to know to come here to read about deephaven.ui? This also won't render and look nice like everything else on the docs site.

@dgodinez-dh
Copy link
Contributor Author

Only one comment for now, since my next questions are what are at the forefront of my mind as I read this.

What's the reason this and other PRs are not in the docs repo? Do we really expect users to know to come here to read about deephaven.ui? This also won't render and look nice like everything else on the docs site.

@dsmmcken Can you reply to our docs strategy?

@dgodinez-dh dgodinez-dh requested a review from jjbrosnan December 9, 2024 21:27
@dsmmcken
Copy link
Contributor

dsmmcken commented Dec 10, 2024

What's the reason this and other PRs are not in the docs repo? Do we really expect users to know to come here to read about deephaven.ui? This also won't render and look nice like everything else on the docs site.

@jjbrosnan It will render in salmon. That's one of the neat design goals behind it. Let's us keep all the docs next to the code where they can stay updated and be used for testing, but still assemble it up into a single docs site.


## Children and props

Arguments passed to a component may be either `children` or `props`. `Children` refers to `child` components that are passed to a `parent` component as positional arguments. `Props` are properties passed as keyword arguments that determine the behavior and rendering style of the component. Positional arguments must be included in the correct order. Keyword arguments are included with a keyword and equals sign.
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting that keyword argument ordering doesn't matter, but that ordering of keyword arguments can affect ordering of positional arguments and make them out of order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added


Arguments passed to a component may be either `children` or `props`. `Children` refers to `child` components that are passed to a `parent` component as positional arguments. `Props` are properties passed as keyword arguments that determine the behavior and rendering style of the component. Positional arguments must be included in the correct order. Keyword arguments are included with a keyword and equals sign.

```python
Copy link
Contributor

Choose a reason for hiding this comment

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

Image of the output?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dsmmcken will this be testable with the snapshot tool if this doesn't live in docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added


In the above example, the `flex` component is the `parent`. It has three `children`: a `heading`, a `button`, and a `text` component. These `children` will be rendered inside the `flex`. It also has three props: `direction`, `wrap`, and `width`. These three props indicate that the flex should be rendered as a 200 pixel column with wrap enabled.

## Comparison with JSX
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the purpose of this section. What component rule does this demonstrate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mofojed Requested a comparison with JSX.

Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose is showing users familiar with React how to modify this for DH, but an intro sentence saying as much would be useful.


To define `children` and `props` for a custom component, add them as arguments to the component function. As a convention, you may declare the children using the `*` symbol to take any number of arguments.

```python
Copy link
Contributor

Choose a reason for hiding this comment

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

An image of the component would be good to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added


## Component return values

A `deephaven.ui` component usually returns a component. It may also return a list or tuple of components. It may return `None` if it should perform logic but does not need to be rendered. It may also return a single value like a `string` or `int`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this differ at all from any other Python UDF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. A Python UDF can return any value. A deephaven.ui component is limited in valid return values.


my_button = return_conditional(True)
my_text = return_conditional(False)
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Related documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can link to conditional rendering. All that section has not been created yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

The plugins / deephaven.ui docs don't have a Related docs section; so either all of them need it, or we agree not to include it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of the docs in this repo will eventually be rendered on the docs site. All of the pages on there have related documentation at the bottom. These should have it.

my_return_int = return_int()
```

## Conditional return
Copy link
Contributor

Choose a reason for hiding this comment

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

This section feels like the same topic as the previous. Any reason not to just combine them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous section is about valid return values. This section is about returning different values based on a condition. I think it is clearer in two different sections.

Copy link
Contributor

@jjbrosnan jjbrosnan left a comment

Choose a reason for hiding this comment

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

Several comments.

@margaretkennedy margaretkennedy changed the title docs: Component Rules DOC-204: Component Rules Dec 10, 2024
@margaretkennedy margaretkennedy changed the title DOC-204: Component Rules docs: Component Rules Dec 10, 2024
plugins/ui/docs/describing/component_rules.md Outdated Show resolved Hide resolved

In the above example, the `flex` component is the `parent`. It has three `children`: a `heading`, a `button`, and a `text` component. These `children` will be rendered inside the `flex`. It also has three props: `direction`, `wrap`, and `width`. These three props indicate that the flex should be rendered as a 200 pixel column with wrap enabled.

## Comparison with JSX
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose is showing users familiar with React how to modify this for DH, but an intro sentence saying as much would be useful.

plugins/ui/docs/describing/component_rules.md Outdated Show resolved Hide resolved

my_button = return_conditional(True)
my_text = return_conditional(False)
```
Copy link
Contributor

Choose a reason for hiding this comment

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

The plugins / deephaven.ui docs don't have a Related docs section; so either all of them need it, or we agree not to include it for now.

plugins/ui/docs/describing/component_rules.md Outdated Show resolved Hide resolved
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.

5 participants