Skip to content

feat: item as title, parent item hide the check input if isnt selectable #136

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

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

Conversation

cr0wg4n
Copy link

@cr0wg4n cr0wg4n commented Sep 1, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • All tests are passing
  • New/updated tests are included

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Hi, The reason why i made this change is because the "checkboxes" in parent items are something ugly and if is not selectionable, the "checkboxes" are only disabled, the parent item in many cases it will be like a title, and this feature makes it possible, the tests are ok and I increase the "Selectable Items Example" in the storybook, thanks a lot!. :D

@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #136   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files           4        4           
  Lines         122      123    +1     
  Branches       27       28    +1     
=======================================
+ Hits          121      122    +1     
  Misses          1        1           
Impacted Files Coverage Δ
src/utils/tree-model.js 98.82% <100.00%> (+0.01%) ⬆️

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 0a737a2...99d8f24. Read the comment docs.

@jledentu
Copy link
Owner

jledentu commented Sep 2, 2020

Hi @cr0wg4n, thanks for your pull request!

I understand what you need. The problem is that your PR introduces a breaking change, since the behaviour you implemented is different than the current one: parent items now don't have checkboxes by default. It will change some behaviours. In my own use case, I want to keep checkboxes on parent items by default.

Also, a checkbox will now appear on items with selectable: true even if the Finder is not itself selectable.

Maybe we could add a new prop hideCheckboxOnUnselectableParentItems on the finder? Or a hideCheckbox property on items?

Another more flexible solution would be to add a checkboxComponent (like itemComponent, arrowComponent...) allowing to customize the checkbox and changing it depending on the item.

@cr0wg4n
Copy link
Author

cr0wg4n commented Sep 2, 2020

"I want to keep checkboxes on parent items by default", thats the problem for this solution right? and well, another component sounds very good and scalable, honestly I don't have much experience with tests, if I make this new idea successfully, can you help me to write tests :D?, thank you for the feedback.

@jledentu
Copy link
Owner

jledentu commented Sep 3, 2020

"I want to keep checkboxes on parent items by default", thats the problem for this solution right? and well, another component sounds very good and scalable, honestly I don't have much experience with tests, if I make this new idea successfully, can you help me to write tests :D?, thank you for the feedback.

@cr0wg4n Sure, feel free to try this development and I'll help you or finish if you need to. You can see tests for arrowComponent as example. Tests will consist in setting the checkboxComponent as prop in the test, and making a snapshot test.

https://jestjs.io/docs/en/snapshot-testing

@cr0wg4n
Copy link
Author

cr0wg4n commented Sep 4, 2020

@jledentu Thank you so much!, I will try

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.

2 participants