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.

Sign up for GitHub

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

Jump to bottom

Adding checkboxes #181

Closed

TKul6 wants to merge 6 commits into valor-software:develop from TKul6:adding-checkboxes

TKul6 commented Nov 25, 2017

Hi,

I almost completed the checkboxes feature and I wanted your opinion (to see if there any necessary changes I should do.)

I'm still experiencing some difficulties with implementing the Indeterminated state of a checkbox. However I wanted to start the merge process to see that everything else is fine.

As soon as you give me the O.K. I will add tests and documentation.

Thanks,
TMaster.

TMaster and others added 4 commits

November 17, 2017 14:17


          Support checking / unchecking nodes

4a68b64


          Support readonly checkboxes

e255869


          Merge branch 'develop' into adding-checkboxes

1bfe991


          Fixing merge issues

codecov bot commented Nov 25, 2017 •

edited

Loading

Codecov Report

Merging #181 into develop will decrease coverage by 5.52%.
The diff coverage is 43.75%.

@@             Coverage Diff             @@
##           develop     #181      +/-   ##
===========================================
- Coverage    95.86%   90.34%   -5.53%     
===========================================
  Files           23       23              
  Lines          823      911      +88     
  Branches       120      130      +10     
===========================================
+ Hits           789      823      +34     
- Misses          16       70      +54     
  Partials        18       18

Impacted Files	Coverage Δ
src/tree.types.ts	`100% <ø> (ø)`	⬆️
src/tree.ts	`93.11% <ø> (ø)`	⬆️
src/tree.component.ts	`90.9% <100%> (+0.71%)`	⬆️
src/tree-internal.component.ts	`70.16% <31.34%> (-23.12%)`	⬇️
src/tree-controller.ts	`96.22% <50%> (-3.78%)`	⬇️
src/tree.service.ts	`96% <66.66%> (-4%)`	⬇️
src/tree.events.ts	`84.78% <66.66%> (-4.41%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0aab34...f62e3b5. Read the comment docs.

Contributor

rychkog commented Nov 25, 2017

@Tmaster Cool, thanks! I don't have a chance to dive into this feature during these weekends but I'll start looking at it on Monday and give you my feedback ASAP.

TMaster added 2 commits

December 2, 2017 13:02


          Support indeterminated state

92a96af


          Start comment with a space

f62e3b5

Author

TKul6 commented Dec 2, 2017

HI @rychkog
I finished the indeterminate feature for the tree, you can check it out.

However there is still a little problem and I'm not sure about my solution.
While developing the feature I needed to check for a parent if one of it's children are checked, and validated it using the node id's. After a while I realized the node doesn't have to have an id which make a crazy behavior when checking a node with no id.

I temporary overcame this problem by checking if the node has a defined id, and if it doesn't I send a message to the console.

How do you recommend to solve this problem?

Thanks,
TMaster

And again, . . I'll add my tests after you approve the feature.

TKul6 mentioned this pull request

Allows checkbox for tree nodes #79

Closed

Contributor

rychkog commented Feb 1, 2018

@Tmaster Sorry for being silent so long - has lots of work without a chance to dedicate some time to open-source.

Contributor

rychkog commented Feb 1, 2018

I hope you still believe in a bright future of this project :)

rychkog suggested changes

View reviewed changes

src/tree-internal.component.ts

+                private updateIndeterminateStateInternal(): void {
+                  const checkedChildren = this.tree.children.filter(child => child.isChecked).length;
+                  if (checkedChildren === 0) {

This comment was marked as off-topic.

Sign in to view

src/tree-internal.component.ts

                 public isSelected = false;
                 public isRightMenuVisible = false;
                 public isLeftMenuVisible = false;
+                public isChecked = false;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

src/tree-internal.component.ts

                 public controller: TreeController;
+                @ViewChild('checkbox')
+                _checkboxElement: ElementRef;

This comment was marked as off-topic.

Sign in to view

src/tree-internal.component.ts

-                                   public treeService: TreeService,
-                                   public element: ElementRef) {
+                  public treeService: TreeService,
+                  public element: ElementRef) {

This comment was marked as off-topic.

Sign in to view

src/tree-internal.component.ts

                     this.treeService.setController(this.tree.node.id, this.controller);
                   }
-                  this.settings = this.settings || { rootIsVisible: true };
+                  this.settings = this.settings || { rootIsVisible: true, showCheckboxes: false, enableCheckboxes: true };

This comment was marked as off-topic.

Sign in to view

Contributor

rychkog commented Feb 1, 2018

@Tmaster Had a quick look and overall it is cool - will review in a bit more holistic manner today evening (my time - Eastern European Time (Standard Time))

rychkog suggested changes

View reviewed changes

src/tree-internal.component.ts

                 public isSelected = false;
                 public isRightMenuVisible = false;
                 public isLeftMenuVisible = false;
+                public isChecked = false;

This comment was marked as off-topic.

Sign in to view

rychkog pushed a commit that referenced this pull request


          feat(tree): add checkboxes support (closes #181, closes #79)

e452727

rychkog mentioned this pull request

Add checkboxes support #211

Merged

rychkog pushed a commit that referenced this pull request


          feat(tree): add checkboxes support (closes #181, closes #79)

Contributor

rychkog commented Feb 11, 2018

@Tmaster I've added tests, fixed some issues in here #211. So will close this one.

rychkog closed this

Author

TKul6 commented Feb 11, 2018

Thanks a lot @rychkog .
Sorry about the delay, I have some things I have to close at work. no time for open source :(

Contributor

rychkog commented Feb 11, 2018

No worries - same stuff on my side :)

longgt pushed a commit to longgt/ng2-tree that referenced this pull request


          feat(tree): add checkboxes support (closes valor-software#181, closes v…

7d74563

…alor-software#79)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet