-
Notifications
You must be signed in to change notification settings - Fork 208
feat: Make split panel visible when collapsed at the bottom in new toolbar layout #3086
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3086 +/- ##
=======================================
Coverage 96.34% 96.34%
=======================================
Files 779 778 -1
Lines 21913 21910 -3
Branches 7443 7453 +10
=======================================
- Hits 21112 21110 -2
+ Misses 749 748 -1
Partials 52 52 ☔ View full report in Codecov by Sentry. |
efa73c0 to
2c6f0b5
Compare
231d11f to
c627936
Compare
fdb7f9c to
783ac35
Compare
| test( | ||
| 'Shows tooltip correctly for split panel trigger for keyboard (tab) interactions', | ||
| setupTest({ theme, size, splitPanelPosition }, async (page: AppLayoutDrawersPage) => { | ||
| await expect(page.isExisting(tooltipSelector)).resolves.toBe(false); |
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 path taken by these tests to focus the split panel trigger in the toolbar was not working anymore because it was assuming that the focus stays on the trigger after clicking it, but now it moves to the split panel.
|
This change broke split panel slide in animation. It works fine in the mainline and does not on your PR preview build: https://d21d5uik3ws71m.cloudfront.net/components/783ac3514ee5eb19adb067e5c023a114f456691e/dev-pages/index.html#/light/app-layout/with-split-panel?appLayoutToolbar=true |
This reverts commit 1d1f3db.
783ac35 to
c074481
Compare
Thanks for noticing! This was addressed in the latest commit (c074481). |
| await page.keys(['Tab', 'Tab']); // Focus the first drawer trigger past the split panel trigger | ||
| await page.keys(['Shift', 'Tab']); // Tab back to the split panel trigger |
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.
Do we need to do this back-and-forward focus in every test? Is it really worth reusing this code? For example, you could only reuse the assertion part.
| testutilStyles['drawers-trigger'], | ||
| splitPanelTestUtilStyles['open-button'] | ||
| testutilStyles['split-panel-trigger'], | ||
| splitPanelResolvedPosition === 'side' && splitPanelTestUtilStyles['open-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.
Let's keep this test util class assigned in both cases.
The collapsed split panel will not use this class. All customers' tests will open split panel using this button in every position
src/split-panel/bottom.tsx
Outdated
|
|
||
| useResizeObserver(headerRef, entry => { | ||
| const { borderBoxHeight } = entry; | ||
| closedPanelBlockSize.current = `calc(${borderBoxHeight}px + ${tokens.borderPanelTopWidth})`; |
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.
This line does not look like idiomatic React code. What is happening here?
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 am using closedPanelBlockSize.current as blockSize in line 86 in order to keep the animated transition. This gives the element the same height that it would already have without applying any value, but a value needs to be explicitly set because you cannot transition from or to undefined; a value has to be defined for the transitioned property in both states.
What do you find most non-idiomatic here? Do you have any suggestion? Or does it look better knowing this information?
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.
For idiomatic code, it should be a state, not ref. Also, can we reuse reportHeaderHeight somehow which already sets the state?
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 can pass down reportHeaderHeight from the app layout, although it is only needed for the toolbar layout, so either we have to make it optional in the split panel context, or we pass it anyway in the three app layouts, whether it is actually used in the split panel or not.
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.
Okay, let's introduce our own local state then.
Also see cloudscape-design/component-toolkit#107, I added optimization for multiple state changes in this case
c074481 to
bb22459
Compare
Description
Issue:
AWSUI-60031Now the split panel in the new toolbar layout behaves the same as in the old layout when it is displayed at the bottom:
findSplitPanelOpenButtontest util will return the open button in the split panel and not the toolbar split panel triggerHow has this been tested?
7138307483)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.