fix(headercell): added new hover prop to control component hover state #928
Conversation
| @@ -55,28 +57,36 @@ export const StyledTableHeader = styled("th", { | |||
| ${focusVisibleA11yStyle} | |||
|
|
|||
| ${(props: CellHeaderExtraProps) => { | |||
| const { active = false, horizontalAlign = "left" } = props; | |||
| const { active = false, horizontalAlign = "left", hover = true } = props; | |||
There was a problem hiding this comment.
It looks like it defaults to "true" 👍
There was a problem hiding this comment.
Is it ok? or should it be the other way around? 🤔
What do you think @clarsen-czi?
There was a problem hiding this comment.
@clarsen-czi sorry I think we still need your feedback here 😁 🙏
If we default hover to true, it would mean that the CellHeader could have hover effect when no corresponding action is present/implemented, so the question here is whether we should default hover to false instead to prevent that?
Thank you!
There was a problem hiding this comment.
Good call @tihuan! I agree that having it default to false makes the most sense. Thanks for the suggestion!
There was a problem hiding this comment.
LGTM!! Thanks for jumping on this so quickly @masoudmanson!
masoudmanson
deleted the
927-add-hover-prop-to-the-headercell-component
branch
|
Yes! Thanks Masoud for the quick update, Erin for raising the default and Connor for confirming it! 🎉 |
Summary
HeaderCell
Github issue: #927
https://czi-sci.slack.com/archives/C032S43KKFV/p1738019694310679
Checklist
defaultTheme.tsused wherever possible