fix(Section): defaultIsOpen should behave as expected #6326
Conversation
|
Thanks for your interest in palantir/blueprint, @nerdstep! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
nerdstep
changed the title
| @@ -109,7 +112,7 @@ export const Section: React.FC<SectionProps> = React.forwardRef((props, ref) => | |||
| title, | |||
| ...htmlProps | |||
| } = props; | |||
| const [isCollapsed, setIsCollapsed] = React.useState<boolean>(collapseProps?.defaultIsOpen ?? false); | |||
| const [isCollapsed, setIsCollapsed] = React.useState<boolean>(!collapseProps?.defaultIsOpen ?? false); | |||
There was a problem hiding this comment.
The ! turns collapseProps?.defaultIsOpen into a boolean. Is ?? false needed if the left side will never be undefined or null?
It is a bit verbose, but I think collapseProps?.defaultIsOpen ?? true conveys the new @default true pretty well. Perhaps we should invert after performing that defaulting logic?
| const [isCollapsed, setIsCollapsed] = React.useState<boolean>(!collapseProps?.defaultIsOpen ?? false); | |
| const [isCollapsed, setIsCollapsed] = React.useState<boolean>(!(collapseProps?.defaultIsOpen ?? true)); |
There was a problem hiding this comment.
Agreed, that does make for more readable code! Updated the PR with the suggested change, including a comment to better explain the behavior. Also added a new test to assert the defaultIsOpen={undefined} case.
| // re-mount when `defaultIsOpen` is changed, otherwise | ||
| // the local state in the `Collapse` component is not | ||
| // updated. | ||
| key={String(defaultIsOpen)} |
There was a problem hiding this comment.
Using key to reset state and animations is one of my favorite tricks in the pocket too. 🙂
There was a problem hiding this comment.
Haha yeah it's a neat little trick! :)
Fixes #6322
Fixes #0000
Checklist
Changes proposed in this pull request:
Negate
defaultIsOpenvalue provided touseStateso that it matches the expectedisCollapsedbehavior.Reviewers should focus on:
Whether this fixes the desired behavior.
Screenshot
when
defaultIsOpen={true}when
defaultIsOpen={false}