-
Notifications
You must be signed in to change notification settings - Fork 208
feat: Add panel-layout component #4114
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
Conversation
14dfc75 to
5b0044a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4114 +/- ##
==========================================
+ Coverage 97.12% 97.14% +0.01%
==========================================
Files 865 869 +4
Lines 25362 25464 +102
Branches 9155 9209 +54
==========================================
+ Hits 24634 24736 +102
Misses 681 681
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pages/panel-layout/nested.page.tsx
Outdated
| } = useContext(AppContext as PageContext); | ||
|
|
||
| return ( | ||
| <I18nProvider messages={[messages]} locale="en"> |
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.
nit: There is now a helper component SimplePage for rendering test pages w/o app layout. It includes i18n and screenshot area:
<SimplePage title="Nested Panel Layout Demo" i18n={{}} screenshotArea={{}}>
<>...</>
</SimplePage>
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.
oh, and it has a slot for settings
57e4609 to
1a8c2e1
Compare
| <Box variant="h2" padding={{ bottom: 'm' }}> | ||
| Artifact | ||
| </Box> | ||
| <Button |
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.
nit: There is no margins between these buttons.
| </Box> | ||
| ); | ||
| return ( | ||
| <div ref={ref} style={{ width: '100%', overflow: 'hidden' }}> |
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.
nit: I don't mind inline styles, but since we already use class names e.g. for the ai-artifact-panel, it would be nice to use it here, too - for consistency
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.
styles have been removed in latest version
pages/panel-layout/nested.page.tsx
Outdated
| } = useContext(AppContext as PageContext); | ||
|
|
||
| return ( | ||
| <I18nProvider messages={[messages]} locale="en"> |
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.
oh, and it has a slot for settings
| }: PanelLayoutContentProps) => { | ||
| const [size, setSize] = useState(Math.max(200, minPanelSize)); | ||
|
|
||
| const [_actualMaxPanelSize, ref] = useContainerQuery( |
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.
The onPanelResize.detail does include totalSize, but it looks like we cannot use it as the callback only fires on user-issued resize. Would it make senes to maybe add another callback that fires when the panel size changes?
We already use the container query inside the component, and if we expose the callback - the consumers won't have to use another container query outside.
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.
Nice idea, it also gets rid of a lot of places where teams have to add styled containers (because they no longer need a parent element to attach a ref to). Adding as onLayoutChange
pages/panel-layout/nested.page.tsx
Outdated
| ); | ||
|
|
||
| return ( | ||
| <div style={{ height: '100%', overflow: 'hidden' }}> |
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.
I noticed that we add these styles to all test pages. Would it make sense to support that on the component itself with props? E.g. some stretchWidth and stretchHeight flags?
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.
It's not actually necessary on this dev page, will remove it. Whether (and what exactly) this styling is necessary is very dependent on the specific use-case, so I don't see a strong case for adding this as I think it would end up adding a fair amount of complexity to the API to handle multiple different cases.
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.
Ok, it's fair. Let's then remove it from test pages where it is not needed
| onPanelResize={({ detail }) => setSize(detail.panelSize)} | ||
| display={display} | ||
| panelPosition={panelPosition} | ||
| mainFocusable={longMainContent && !buttons ? { ariaLabel: 'Main content' } : undefined} |
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.
nit: logically it probably makes more sense to only depend on !buttons, as unless the content is measured or expected to be very small - there is a chance that it will not fit the screen when the viewport is small.
pages/panel-layout/nested.page.tsx
Outdated
| </SpaceBetween> | ||
| </Container> | ||
| } | ||
| mainFocusable={{ ariaLabel: 'Level 2 main' }} |
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.
I also wonder, if it is possible to implement the panel in such a way that the header is sticky (or effectively sticky), so that only the content part is scrollable. It not or if hard - it might make sense to offer separate slots for header and content, potentially.
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.
Pushed an update that seems to work better to me: it's no longer obscured by the scrollbars. I still don't have a solution for the scenario on the nested page (other than adding padding), but I also think in that scenario (as mentioned in a thread above) the ideal solution would be scrolling inside the container which currently would need to be implemented inside the panel contents (i.e. by the consumer), so I think this is a reasonable solution to the most common use-cases
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.
I agree, when a container-like content is added - having the scroll outside does not look right. Is it straightforward to implement an internal scroll, though? There might be challenges are the panels already have overflow styles, added unconditionally.
|
|
||
| export interface PanelResizeDetail { | ||
| totalSize: number; | ||
| panelSize: number; |
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 main content size be obtained as totalSize - panelSize, or one must account for the resize handle when resizable=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.
panelSize includes the resize handle, so actual available panel content size is smaller if resizable
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.
Should we compensate for that?
E.g. if I have some content which min size is precisely 400px - I should want to know that it can fit into the panel. Unless we do this - the consumer will have to account for the resize handle size, which would require to check for resizable and display props also to figure out if the resize handle size should be subtracted from the panel size.
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.
As we know when the resize handle is shown and its size, we can do the compensation, and then expose totalSize, panelSize, and mainSize in the event handler details (in which case the sum of panel and main size will only equal total size if there is no resize handle).
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.
I think that makes it actually harder for teams. For example, the main use-case for that callback is adjusting the maxPanelSize to have an effective minContentSize. But for that to work properly the values have to add up consistently, and if we're compensating for the handle I can't see a way that we can do that that will be easy to understand.
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 we expose an additional panelContentSize (that is panel size minus handle size)?
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.
That can actually be an incremental change, unlike the one I suggested earlier - so we can do it later as well.

Description
Create new "panel layout" component
Related links, issue #, if available: AWSUI-61325
How has this been tested?
Added unit and integ tests, will add screenshot tests separately
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.