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

Tree widget: Status of consuming Kiwi UI #1149

Open
11 of 28 tasks
MartynasStrazdas opened this issue Jan 24, 2025 · 12 comments
Open
11 of 28 tasks

Tree widget: Status of consuming Kiwi UI #1149

MartynasStrazdas opened this issue Jan 24, 2025 · 12 comments
Assignees
Labels
tree widget Tree widget or its components related issues

Comments

@MartynasStrazdas
Copy link
Contributor

MartynasStrazdas commented Jan 24, 2025

Current widget status

A short demo of the Kiwi-based tree widget at the current stage: https://github.com/user-attachments/assets/5a157d8c-c9a2-4410-9c74-eb331e7ae8d8

Current kiwi tree implementation: iTwin/presentation#847

iTwinUI v5 dependencies status

Below is a list of iTwinUI v5 components that have been successfully consumed or are still missing (based on status). The list is incomplete, more components / issues might appear in the future.

Components required for Tree rendering

Preview Give feedback

Components required for Tree widget

Preview Give feedback

Icons status

Tasks

Preview Give feedback

Mockups status

Tasks

Preview Give feedback
@grigasp grigasp changed the title Tree-widget missing kiwi components/functionalities Tree widget: Status of consuming Kiwi UI Jan 24, 2025
@grigasp grigasp added the tree widget Tree widget or its components related issues label Jan 24, 2025
@GerardasB
Copy link
Contributor

Tree item equivalent error node component.

Would you like to display an error state for specific tree items? Or are you looking for an error state for the whole tree i.e. instead of displaying any nodes, it could say that the tree is empty or that there was a problem fetching data.

Multiline label

Can we simply expose a sub-label prop or would you like to update the styling of existing label i.e. to specify https://developer.mozilla.org/en-US/docs/Web/CSS/text-wrap?

Tree item expander click selects tree item (unconfirmed, might be our issue)

Are you using onExpandedChange? Seems to work fine in our sandbox w/o additional custom logic.

Unified onClick and onKeyDown for item selection (nice to have)

We have onSelectedChange for that use-case. But we are not handling multi selection (or key modifiers).

Dropdown menu button

You can use a DropdownMenu component.

Search box

Take a look at TextBox and IconButton components. I.e. you could create a simple search box by simply using decorations API:

<TextBox.Root>
	<TextBox.Icon href={searchIcon} />
	<TextBox.Input placeholder="Search" />
</TextBox.Root>
<IconButton
	icon={searchIcon}
	label="Search"
	variant="ghost"
/>

Progress radial (indeterminate)

This is available as a Spinner component.

Checkbox to have a partial state

We support "mixed" state in the Checkbox component. But I believe, there's a rendering issue, we'll investigate.

@MartynasStrazdas
Copy link
Contributor Author

MartynasStrazdas commented Jan 27, 2025

Tree item equivalent error node component.

Would you like to display an error state for specific tree items? Or are you looking for an error state for the whole tree i.e. instead of displaying any nodes, it could say that the tree is empty or that there was a problem fetching data.

Multiline label

Can we simply expose a sub-label prop or would you like to update the styling of existing label i.e. to specify https://developer.mozilla.org/en-US/docs/Web/CSS/text-wrap?

Tree item expander click selects tree item (unconfirmed, might be our issue)

Are you using onExpandedChange? Seems to work fine in our sandbox w/o additional custom logic.

Unified onClick and onKeyDown for item selection (nice to have)

We have onSelectedChange for that use-case. But we are not handling multi selection (or key modifiers).

Dropdown menu button

You can use a DropdownMenu component.

Search box

Take a look at TextBox and IconButton components. I.e. you could create a simple search box by simply using decorations API:

<TextBox.Root>
<TextBox.Icon href={searchIcon} />
<TextBox.Input placeholder="Search" />
</TextBox.Root>

Progress radial (indeterminate)

This is available as a Spinner component.

Checkbox to have a partial state

We support "mixed" state in the Checkbox component. But I believe, there's a rendering issue, we'll investigate.

@GerardasB

  • Tree item equivalent error node component.
    The error/warning would be for specific tree items, ex. we expand a node and the node has lots of children, more than 1k, in this case we would display a warning node that would say to filter the nodes instead.

  • Multiline label
    I think sublabel is enough there has not been a need for styling.

  • Tree item expander click selects tree item (unconfirmed, might be our issue)
    Yes we are using onExpandedChange, but our selection is based on onClick and onKeyDown, which I think is the issue as expandedChange does not stopPropagation.

  • Unified onClick and onKeyDown for item selection (nice to have)
    This is more for the future when you work on keyboard navigation as in previous tree we had to check if the target is the node when using keyboard, it would be nice to have one callback for both mouse and keyboard.

  • Search box
    For the future do you have plans for search box?

  • Progress radial (indeterminate)
    I know spinner exists, but in this component, we would prefer a line-based progress bar.

@mayank99
Copy link

@MartynasStrazdas,

  • Tree item equivalent error node component.
    The error/warning would be for specific tree items, ex. we expand a node and the node has lots of children, more than 1k, in this case we would display a warning node that would say to filter the nodes instead.

We'll keep this in mind for implementing in the near future. I saw the design for it, but I feel unsure about the API and semantics right now.

  • Tree item expander click selects tree item (unconfirmed, might be our issue)
    Yes we are using onExpandedChange, but our selection is based on onClick and onKeyDown, which I think is the issue as expandedChange does not stopPropagation.
  • Unified onClick and onKeyDown for item selection (nice to have)
    This is more for the future when you work on keyboard navigation as in previous tree we had to check if the target is the node when using keyboard, it would be nice to have one callback for both mouse and keyboard.

You should really be using onSelectedChange instead of adding your own onClick and onKeyDown handlers.

In the next release, we'll be adding full support for keyboard navigation, so hopefully you won't have any need to reach for DOM events. We'll make sure there is no propagation issue.

Multi-selection is not going to be handled entirely by this prop. The purpose of onSelectedChange is to abstract away the interactions (clicks and keyboard events) and give you a single place to handle your own selection logic.

  • Search box
    For the future do you have plans for search box?

iTwinUI v3 offers a SearchBox, so it makes sense to add in v5 as well. As @GerardasB mentioned, we currently recommend using TextBox (it supports decorations). There isn't a big difference between the two components.

  • Progress radial (indeterminate)
    I know spinner exists, but in this component, we would prefer a line-based progress bar.

It looks like you are asking for ProgressLinear, not ProgressRadial. We'll make a note of this, but in the meantime you can still use Spinner.

@grigasp
Copy link
Member

grigasp commented Jan 28, 2025

You should really be using onSelectedChange instead of adding your own onClick and onKeyDown handlers.

In the next release, we'll be adding full support for keyboard navigation, so hopefully you won't have any need to reach for DOM events. We'll make sure there is no propagation issue.

Multi-selection is not going to be handled entirely by this prop. The purpose of onSelectedChange is to abstract away the interactions (clicks and keyboard events) and give you a single place to handle your own selection logic.

@mayank99, just to clarify - does this mean you're going to support multi-selection out of the box in your tree component?

@mayank99
Copy link

@grigasp, Maybe not immediately but yes I think we should support multi-selection (including ctrl and shift key combinations) in our Tree. For now, we can expose the original event through onSelectedChange.

Unrelated: Could you or @MartynasStrazdas clarify what is meant by "ButtonGroup or equivalent component"? Where is it needed (perhaps the actions inside the tree, or the toolbar above the tree)? And what kind of "overflow logic" do you expect from this component?

@aruniverse
Copy link
Member

Unrelated: Could you or @MartynasStrazdas clarify what is meant by "ButtonGroup or equivalent component"? Where is it needed (perhaps the actions inside the tree, or the toolbar above the tree)? And what kind of "overflow logic" do you expect from this component?

Reviewing what the existing trees look like might give you some idea.
https://github.com/iTwin/viewer-components-react/tree/master/packages/itwin/tree-widget
You can also play around with it in any of our viewer based apps.

@grigasp
Copy link
Member

grigasp commented Jan 29, 2025

Unrelated: Could you or @MartynasStrazdas clarify what is meant by "ButtonGroup or equivalent component"? Where is it needed (perhaps the actions inside the tree, or the toolbar above the tree)? And what kind of "overflow logic" do you expect from this component?

We need it for the tree header:
Image

I'd say this is low priority at the moment, because it's still not in the mockups, so it may turn out that we don't need it.

@aruniverse
Copy link
Member

I pulled in the first alphas release of the tree using kiwi into my test app.
If you click on the kebab menu in the header there will be some feature flag toggles you can enable to see the tree,

Image
Since this is still very early and we are looking to obtain feedback, I imagine most consumers for now will want to have the option to display both v3 and vAlpha. I might recommend temporarily modifying the id in createTreeWidget so you can have multiple TreeWidget UiProvider instances.

It also might help to temporarily wrap the <TreeWidgetComponent /> with the kiwi <Root> ThemeProvider just to help teams pull it in for testing. This might also alleviate the need for having the peer dep on itwinui-react@v5 which you need to add some hacks to properly resolve if you have multiple versions of iTwinUi installed.

@mayank99
Copy link

mayank99 commented Feb 4, 2025

When playing with the tree widget, I noticed some visual and interaction issues (which you might already be aware of):

  • The Select at the top of the widget should take up the full width of the widget (you might need to style Select.HtmlSelect).
  • The IconButtons in the toolbar above the tree should be using variant="ghost" and should be aligned to the left side (justify-content: flex-start).
    • (In the future, Kiwi will likely provide a "toolbar" component that handles the base spacing as well as overflow logic)
  • The Tree should fill the remaining height of the widget, so that a horizontal scrollbar does not appear in the middle of the widget.
  • Selection behavior feels inconsistent/confusing. See video, described below.
    • Clicking a tree-node selects it. Clicking a second tree-node does not select it; it only deselects the first one. I have to click it again to select it. But then I cannot click it again to deselect it.
    • Clicking on some parent nodes ("Physical Object" in video) selects its child nodes, but in other cases ("Default" in video) it only selects the parent node. (This might be intentional; just wanted to share my observation).

@MartynasStrazdas
Copy link
Contributor Author

MartynasStrazdas commented Feb 5, 2025

When playing with the tree widget, I noticed some visual and interaction issues (which you might already be aware of):

  • The Select at the top of the widget should take up the full width of the widget (you might need to style Select.HtmlSelect).

  • The IconButtons in the toolbar above the tree should be using variant="ghost" and should be aligned to the left side (justify-content: flex-start).

    • (In the future, Kiwi will likely provide a "toolbar" component that handles the base spacing as well as overflow logic)
  • The Tree should fill the remaining height of the widget, so that a horizontal scrollbar does not appear in the middle of the widget.

  • Selection behavior feels inconsistent/confusing. See video, described below.

    • Clicking a tree-node selects it. Clicking a second tree-node does not select it; it only deselects the first one. I have to click it again to select it. But then I cannot click it again to deselect it.
    • Clicking on some parent nodes ("Physical Object" in video) selects its child nodes, but in other cases ("Default" in video) it only selects the parent node. (This might be intentional; just wanted to share my observation).

Thanks for feedback.

  • The Select at the top of the widget should take up the full width of the widget (you might need to style Select.HtmlSelect).
    Currently we are still waiting for tree header full mockup, we'll fix it when we have it.

  • The IconButtons in the toolbar above the tree should be using variant="ghost" and should be aligned to the left side (justify-content: flex-start).
    The allignment seems to be the apps issue. Will adjust the buttons to ghost variant.

  • The Tree should fill the remaining height of the widget, so that a horizontal scrollbar does not appear in the middle of the widget.
    Looking into it.

  • Selection behavior feels inconsistent/confusing. See video, described below.
    We're looking into it, but seems to be that particular test-app issue.

@MartynasStrazdas
Copy link
Contributor Author

@mayank99 @GerardasB Been looking into the mockups for the tree, in the warning/error nodes part the warning text has color, how should we handle it on our side: will there be colors provided from kiwi side? Will there be an option for text component to have color/variant, or will we have to pick the color ourselves?

@mayank99
Copy link

mayank99 commented Feb 5, 2025

Been looking into the mockups for the tree, in the warning/error nodes part the warning text has color, how should we handle it on our side: will there be colors provided from kiwi side? Will there be an option for text component to have color/variant, or will we have to pick the color ourselves?

@MartynasStrazdas We do have a <Description> component which should have the colors pre-applied, however the <Tree.Item> does not currently provide a way to render custom JSX inside it.

We are working on a proper API for displaying errors in tree. In the meantime, my suggestion would be to apply custom CSS. You can use the --kiwi-color-text-attention-base variable.

Example code
<Tree.Item
  className="my-treeitem-error"
  label="The hierarchy exceeds 1000 items. Provide additional filtering or increase the limit."
/>
/* TODO: remove when Kiwi Tree has built-in support for displaying errors */
.my-tree-item-error > span {
  color: var(--kiwi-color-text-attention-base);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tree widget Tree widget or its components related issues
Projects
None yet
Development

No branches or pull requests

5 participants