-
Notifications
You must be signed in to change notification settings - Fork 66
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
LG-4681: fix row background-color styles #2560
LG-4681: fix row background-color styles #2560
Conversation
|
Size Change: +89 B (+0.01%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
🙏🏽 |
className, | ||
)} | ||
isDisabled: disabled, | ||
isExpanded: (isExpanded && hasSubRows) || isParentExpanded, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shaneeza I updated this to match what Robert mentioned earlier. Can you confirm this makes sense / looks good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remind me again what this is updating?
Also, we are already checking if there are subrows in our custom getRowCanExpand
function inside useLeafyGreenTable
:
leafygreen-ui/packages/table/src/useLeafyGreenTable/useLeafyGreenTable.tsx
Lines 105 to 107 in a966835
getRowCanExpand: row => { | |
return !!row.original.renderExpandedContent || !!row.subRows?.length; | |
}, |
The docs go into a little more detail.
This means a row won't expand if there are no subrows or expandedContent so I don't think we need this subrows check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. So switching the styles around broke this, but adding the subrow check fixes it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think this might have been missing from the initial implementation. Only added this because Robert pointed it out yesterday during our call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stephen and I chatted in person to clarify: when expandedContent is open, the parent row should not have the gray highlight.
✍️ Proposed changes
Refactors row styles and somehow fixes the styling issue (unclear what actually fixed it... 🙃)
🎟 Jira ticket: LG-4681
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changes🧪 How to test changes
Table/Row
Table/WithVirtualizedScrolling