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]: initial version for Tree widget consuming kiwi #1169

Open
wants to merge 24 commits into
base: tree-widget/next
Choose a base branch
from

Conversation

MartynasStrazdas
Copy link
Contributor

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.

@grigasp
Copy link
Member

grigasp commented Jan 30, 2025

I created a tree-widget/next branch for until the tree widget is ready for a full major release. Please re-target your PR to that.

@MartynasStrazdas MartynasStrazdas changed the base branch from master to tree-widget/next January 30, 2025 14:14
Copy link
Member

@grigasp grigasp left a 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?

@MartynasStrazdas
Copy link
Contributor Author

Why did lockfiles of unrelated packages changed? Can we revert those changes?

We added pnpmfile to handle itwinUI v5 peer dependency, still looking if there's a workaround of it not being in root.

Copy link
Member

@saskliutas saskliutas 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. I would remove all the commented out code and props for now and track those removed features in an issue.

@MartynasStrazdas
Copy link
Contributor Author

Why did lockfiles of unrelated packages changed? Can we revert those changes?

We added pnpmfile to handle itwinUI v5 peer dependency, still looking if there's a workaround of it not being in root.

Fixed

@MartynasStrazdas MartynasStrazdas marked this pull request as ready for review January 31, 2025 14:21
@MartynasStrazdas MartynasStrazdas requested review from a team as code owners January 31, 2025 14:21
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

@saskliutas
Copy link
Member

@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).

@grigasp
Copy link
Member

grigasp commented Jan 31, 2025

@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.

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.

3 participants