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

feat: Create tree component #999

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarkoOleksiyenko
Copy link
Contributor

Before submitting the PR, please make sure you do the following:

  • Read the Contributing Guidelines
  • Reference your PR to an issue that was discussed ahead of time
  • The PR should contain a clear description of the problem it solves
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the unit tests with npm run test
  • Run the linting with npm run lint
  • Run the e2e tests with npm run e2e

Thank you for contributing to AgnosUI!

@MarkoOleksiyenko MarkoOleksiyenko force-pushed the tree-component branch 3 times, most recently from ab1a56b to 8ff4407 Compare November 4, 2024 14:49
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 86.46617% with 18 lines in your changes missing coverage. Please review.

Project coverage is 91.82%. Comparing base (de19c38) to head (6f9a977).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
core/src/components/tree/tree.ts 86.95% 4 Missing and 8 partials ⚠️
...ar/bootstrap/src/components/tree/tree.component.ts 72.22% 5 Missing ⚠️
core-bootstrap/src/components/tree/tree.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #999      +/-   ##
==========================================
- Coverage   92.11%   91.82%   -0.29%     
==========================================
  Files          94       98       +4     
  Lines        2535     2668     +133     
  Branches      421      438      +17     
==========================================
+ Hits         2335     2450     +115     
- Misses        130      140      +10     
- Partials       70       78       +8     
Flag Coverage Δ
e2e 83.20% <86.46%> (+0.17%) ⬆️
unit 84.42% <55.43%> (-1.38%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@ExFlo
Copy link
Contributor

ExFlo commented Nov 5, 2024

Hi the screenreader is not fully working when expanding / collapsing the node.

Copy link
Contributor

@ExFlo ExFlo left a comment

Choose a reason for hiding this comment

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

Fix the keyboard navigation with screenreader

angular/bootstrap/src/components/tree/tree.component.ts Outdated Show resolved Hide resolved
core/src/components/tree/tree.ts Outdated Show resolved Hide resolved
core/src/components/tree/tree.ts Outdated Show resolved Hide resolved
$au-tree-expand-button-border-radius: 0.375rem !default;
$au-tree-expand-button-background-color: transparent !default;
$au-tree-expand-button-background-color-hover: #c5d5f9 !default;
$au-tree-expand-icon-color-default: blue !default; // TODO change to a proper color
Copy link
Contributor

Choose a reason for hiding this comment

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

use proper css var of BS with default value like for the slider


.au-tree-expand-button-placeholder {
display: flex;
width: 2.75rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

no css var ?


.au-tree-expand-button {
position: relative;
width: 2.25rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

attributes: {
'aria-label': computed(() => {
const {item} = treeItemContext$();
return `Toggle ${item.label}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we customize this ?

};

/**
* A functional component that renders a tree item elemen.
Copy link
Contributor

Choose a reason for hiding this comment

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

element

const widget = callWidgetFactory({
factory: createTree,
widgetName: 'tree',
props: {...props},
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you need a getter here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the new update

Comment on lines 169 to 171
const _expandedMap$ = writable(new WeakMap<TreeItem, boolean | undefined>());
const _parentMap$ = writable(new WeakMap<TreeItem, TreeItem | undefined>());
const _htmlElementMap$ = writable(new WeakMap<TreeItem, HTMLElement>());
Copy link
Member

@divdavem divdavem Nov 19, 2024

Choose a reason for hiding this comment

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

As far as I understand:
These three stores seem to never change.
I think you should use simple variables, having stores here does not bring any value.
I think it would make sense to re-initialize those stores just before a new tree traversal, to be sure any previous node is no longer in these WeakMaps.

Copy link
Contributor

@quentinderoubaix quentinderoubaix left a comment

Choose a reason for hiding this comment

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

Some comments, thanks @MarkoOleksiyenko for this new component !

angular/bootstrap/src/components/tree/tree.component.ts Outdated Show resolved Hide resolved
angular/bootstrap/src/components/tree/tree.component.ts Outdated Show resolved Hide resolved
angular/bootstrap/src/components/tree/tree.component.ts Outdated Show resolved Hide resolved
angular/bootstrap/src/components/tree/tree.component.ts Outdated Show resolved Hide resolved
angular/bootstrap/src/components/tree/tree.component.ts Outdated Show resolved Hide resolved
svelte/demo/src/bootstrap/samples/tree/Basic.route.svelte Outdated Show resolved Hide resolved
svelte/demo/src/bootstrap/samples/tree/Basic.route.svelte Outdated Show resolved Hide resolved
Copy link
Contributor

@quentinderoubaix quentinderoubaix left a comment

Choose a reason for hiding this comment

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

Tagged all fixed comments as resolved, thx @MarkoOleksiyenko !

imports: [SlotDirective],
template: ` <ng-template [auSlot]="state.structure()" [auSlotProps]="{state, api, directives}"></ng-template> `,
})
export class TreeComponent extends BaseWidgetDirective<TreeWidget> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some jsdoc to the TreeComponent too.

],
},
];
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarkoOleksiyenko you still kept an unnecessary <> </> around the Tree

@MarkoOleksiyenko MarkoOleksiyenko force-pushed the tree-component branch 2 times, most recently from 93d0b87 to 3c0643e Compare November 22, 2024 16:27
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.

Implement Tree widget
4 participants