-
Notifications
You must be signed in to change notification settings - Fork 34
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]: initial version for Tree widget consuming kiwi #1169
base: tree-widget/next
Are you sure you want to change the base?
Conversation
I created a |
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.
Why did lockfiles of unrelated packages changed? Can we revert those changes?
packages/itwin/tree-widget/src/tree-widget-react/components/TreeSelector.tsx
Outdated
Show resolved
Hide resolved
packages/itwin/tree-widget/src/tree-widget-react/components/trees/common/components/Tree.tsx
Outdated
Show resolved
Hide resolved
packages/itwin/tree-widget/src/tree-widget-react/components/trees/common/components/Tree.tsx
Outdated
Show resolved
Hide resolved
...widget/src/tree-widget-react/components/trees/common/components/TreeNodeVisibilityButton.tsx
Outdated
Show resolved
Hide resolved
.../itwin/tree-widget/src/tree-widget-react/components/trees/common/components/TreeRenderer.tsx
Outdated
Show resolved
Hide resolved
We added pnpmfile to handle itwinUI v5 peer dependency, still looking if there's a workaround of it not being in root. |
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.
Looks good. I would remove all the commented out code and props for now and track those removed features in an issue.
...widget/src/tree-widget-react/components/trees/common/components/TreeNodeVisibilityButton.tsx
Outdated
Show resolved
Hide resolved
Fixed |
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.
Why is this needed? Maybe just remove test
script from tree-widget-react package.json?
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.
Wouldn't we then need to remove it from pipeline and other places, would rather just keep it.
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.
It runs through lage
in pipeline so it should be enough to remove cover
/test
scripts from package.json
@grigasp I think we should disable e2e tests for this branch. At least until we reach some stability. Because I think current changes to test-viewer updates property-grid UI too (colors, font, spacing, etc). |
I think I made them non-required check for PR, so they shouldn't prevent PR from being merged. But yeah, I agree it makes sense to just disable them for now. |
Initial preview of kiwi consumption in Tree widget
Demo:
https://github.com/user-attachments/assets/4cc0d50a-6c18-4709-84e5-5a8e58f72eed
There are some known issues, such as button alignment and item selection on expander click that will be fixed later.