#271: Implement Expand All and Collapse All Buttons #286
Conversation
| width={width} | ||
| height={height} | ||
| css={css` | ||
| height: 20px; | ||
| animation: ${spin} 1.4s linear infinite; | ||
| `} | ||
| > | ||
| <path | ||
| fill={fill} |
There was a problem hiding this comment.
This is the only icon that doesn't specify defaults for width/height/fill. Should we?
There was a problem hiding this comment.
Didn't realize this when copying over from stage, probably should follow the pattern.
packages/ui/src/viewer-table/InteractionPanel/CollapseAllButton.tsx
Outdated
Show resolved
Hide resolved
packages/ui/src/viewer-table/InteractionPanel/ExpandAllButton.tsx
Outdated
Show resolved
Hide resolved
packages/ui/stories/viewer-table/interaction-panel/CollapseAllButton.stories.tsx
Outdated
Show resolved
Hide resolved
| import { useThemeContext } from '../../theme/ThemeContext'; | ||
|
|
||
| interface CollapseAllButtonProps { | ||
| setIsCollapsed: (isCollapsed: boolean) => void; |
There was a problem hiding this comment.
I don't think this needs a function argument since this will only ever send true. I think this can simply be named onClick and be () => void.
There was a problem hiding this comment.
I had it send a boolean state value for the initial load, hence the argument, if we remove that behaviour that was requested in the ticket, including the default state on initial page load, then yes it doesn't need the argument.
| useEffect(() => { | ||
| setIsCollapsed(collapsedOnLoad); | ||
| }, [collapsedOnLoad, setIsCollapsed]); |
There was a problem hiding this comment.
Can you explain why this is needed? Usually a button wouldn't be responsible for the state of other components on load. I think this on load behaviour belongs outside this component.
There was a problem hiding this comment.
"A property should be included to set the default state on initial page load (closedOnLoad or openOnLoad)" is included in the ticket for this component, which is what I was thinking when writing this code. However I think I do agree with you since now that I am implementing the accordion component, this is a state that is handled externally. Your thoughts are appreciated.
There was a problem hiding this comment.
I'll recommend maintaining a healthy dose of scepticism when seeing potentially overreaching implementation details on the tickets, they are written by a less technically inclined individual.
| useEffect(() => { | ||
| setIsCollapsed(expandOnLoad); | ||
| }, [expandOnLoad, setIsCollapsed]); |
There was a problem hiding this comment.
Repeat comment about the button not controlling the state of other components on load.
Summary
Issues
Description of Changes
ExpandAllButtonandCollapseAllButtoncomponents for toggling all items open/closed.Buttoncomponent, slightly refactored from theButtoncomponent from thestagerepositoryLoadingicon to successfully port over theButtoncomponent from stageReadiness Checklist
.env.schemafile and documented in the README