-
Notifications
You must be signed in to change notification settings - Fork 64
feat(component): add enhancements to panel component #1678
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 488f719 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
83ef8bc to
488f719
Compare
488f719 to
d79d8d8
Compare
| ${({ theme, padded }) => | ||
| padded !== true && | ||
| css` | ||
| margin-inline: -${theme.spacing.xLarge}; |
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.
these minus margins are a bit awkward
both the technique, plus they're trying to undo the Panel's padding, without referencing the Panel's padding value
so the values could get out of sync
I'm also guessing that we wouldn't want PanelContents ever being used outside of a Panel
So I'm wondering why not hide it as an implementation detail, rather than expose it as its own component
e.g.
<Panel
header="Panel header"
fullWidthContents
>
<Text>
Hello there
</Text>
</Panel>or with a contents namespace to absorb the other new settings (scroll behaviour etc)
<Panel
header="Panel header"
contents={{ fullWidth: true }}
>
<Text>
Hello there
</Text>
</Panel>| `} | ||
| ${({ theme, padded }) => | ||
| padded !== true && |
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.
we normally just do negative boolean checks like !padded
| height?: string; | ||
| scrollable?: boolean; |
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.
would we ever want to support a scenario where the height is fixed, but scrolling is disabled?
would this just result in content being abruptly cropped and unreachable?
| export interface PanelDropdownProps extends Omit<DropdownProps, 'toggle'> { | ||
| toggle: PanelActionProps; | ||
| } |
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.
why not just stick to DropdownProps entirely? What benefit are we looking for by overwriting the concept of the toggle?
|
|
||
| return ( | ||
| <Flex flexDirection="row"> | ||
| <Flex alignItems="center" flexDirection="row" flexGap="0.5rem" justifyContent="space-between"> |
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.
anything we can do with these 0.5rem magic numbers?
What?
Add enhancements to Panel component
Why?
With the introduction of the Lozenge component, the panel also needs to surface this new feature.
The action property does now also accept dropdown properties.
Besides, a new PanelContents component is introduced to bridge the gap of a few long missing features such as ability to expand to the panel content boundaries horizontally (for tables) and scroller functionality for certain scenarios.
Screenshots/Screen Recordings
Testing/Proof
dev.tsxcode