Skip to content

fix(useExpandable): avoid showing checkbox on item details#124

Draft
LightOfHeaven1994 wants to merge 1 commit into
mainfrom
no-checkbox-on-tree-item-details
Draft

fix(useExpandable): avoid showing checkbox on item details#124
LightOfHeaven1994 wants to merge 1 commit into
mainfrom
no-checkbox-on-tree-item-details

Conversation

@LightOfHeaven1994

Copy link
Copy Markdown
Collaborator

Before:
image

After:

image

@LightOfHeaven1994 LightOfHeaven1994 self-assigned this May 28, 2026
@qltysh

qltysh Bot commented May 28, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

⬆️ Merging this pull request will increase total coverage on main by 0.04%.

Modified Files with Diff Coverage (1)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
src/hooks/useExpandable/helpers.js100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

Comment thread src/hooks/useExpandable/helpers.js Outdated
},
props: (() => {
const detailsRowProps = { ...(item.props || {}) };
delete detailsRowProps['aria-level'];

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hm... I'm not really sure where the 'aria-level' is actually set, but we should rather prevent it from being set in the first place if we can.

Side note: It's better to avoid delete and rather try to filter or use object destruction to remove the offending prop. Even if the object is copied and can easily happen someone in the future changes something and might introduce a bug.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@bastilian agree. I tried to use object destruction but linter was complaining about unused variable 😄 I will take a look

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

In this case i think we can disable the linter for that line. I'll look into disabling that rule everywhere.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@bastilian fixed. I feel we can leave @typescript-eslint/no-unused-vars enabled as it might help to have clean code. so in this case I will just disable next line.

Regarding proper place to fix, I think it's already there. Since tree chopper takes care of building tree and leaves, detailsRowForRule takes care of building row details if I understand correctly.

@LightOfHeaven1994 LightOfHeaven1994 force-pushed the no-checkbox-on-tree-item-details branch from 67552de to 7e14986 Compare May 29, 2026 10:50
},
props: (() => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { 'aria-level': _ariaLevel, ...detailsRowProps } = item.props || {};

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I would still first like to try and find out where we set the aria-level in the first place and not set it for the details row cases.

@LightOfHeaven1994 LightOfHeaven1994 marked this pull request as draft June 2, 2026 12:09
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