fix(useExpandable): avoid showing checkbox on item details#124
fix(useExpandable): avoid showing checkbox on item details#124LightOfHeaven1994 wants to merge 1 commit into
Conversation
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (1)
🛟 Help
|
| }, | ||
| props: (() => { | ||
| const detailsRowProps = { ...(item.props || {}) }; | ||
| delete detailsRowProps['aria-level']; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@bastilian agree. I tried to use object destruction but linter was complaining about unused variable 😄 I will take a look
There was a problem hiding this comment.
In this case i think we can disable the linter for that line. I'll look into disabling that rule everywhere.
There was a problem hiding this comment.
@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.
67552de to
7e14986
Compare
| }, | ||
| props: (() => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const { 'aria-level': _ariaLevel, ...detailsRowProps } = item.props || {}; |
There was a problem hiding this comment.
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.

Before:

After: