feat(#3580): push drawer V2 styling and dynamic edge behavior#3578
feat(#3580): push drawer V2 styling and dynamic edge behavior#3578chrisolsen merged 1 commit intodevfrom
Conversation
✅ Deploy Preview for goa-design-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
28c6d4a to
062c758
Compare
24310ae to
61d1535
Compare
062c758 to
117cb7e
Compare
61d1535 to
4a603de
Compare
117cb7e to
03acc1a
Compare
libs/web-components/src/components/push-drawer/PushDrawerInternal.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/push-drawer/PushDrawerInternal.svelte
Show resolved
Hide resolved
4a603de to
a452848
Compare
045dee9 to
8681365
Compare
87a4bf7 to
72b94bd
Compare
| if (target.scrollTop == 0) { | ||
| _scrollPos = "top"; | ||
| } else if ( | ||
| Math.abs(target.scrollHeight - target.scrollTop - target.offsetHeight) < |
There was a problem hiding this comment.
Would be nice to have a declared var here to know the expression represents.
| export let heading: string = ""; | ||
| export let width: string = "492px"; | ||
| export let version: string | undefined = undefined; | ||
| type VersionType = "1" | "2"; |
There was a problem hiding this comment.
It is nice to keep type declarations together. They should be below the import statements to follow the convention used throughout the code.
| export let width: string = "492px"; | ||
| export let version: string | undefined = undefined; | ||
| type VersionType = "1" | "2"; | ||
| const [Version, validateVersion] = typeValidator("Version", ["1", "2"]); |
There was a problem hiding this comment.
Move this const out lf all the export statements.
| async function checkActionsSlotContent() { | ||
| await tick(); | ||
| _actionsSlotHasContent = !!$$slots.actions; | ||
| function checkActionsSlotContent() { |
There was a problem hiding this comment.
This function should be moved into where the other functions are (I can see that it was here to start with though)
Could you also move the let statements above to line ~28, where the other lets are.
There was a problem hiding this comment.
This function just needs to be moved down after the reactive $: blocks..we can leave it for now.
| }); | ||
|
|
||
| const drawer = result.getByTestId("test-drawer"); | ||
| expect(drawer).toHaveClass("v2"); |
There was a problem hiding this comment.
To better ensure the tests pass, like some of the other tests in this file, the expect should be wrapped in a waitFor, the same is true for line :161
72b94bd to
f18ff47
Compare
|
@chrisolsen Addressed all the feedback. Grouped types, moved the const and lets, added named variables for the scroll checks, and wrapped the test assertions in waitFor. Ready for another look when you get a chance. |
| async function checkActionsSlotContent() { | ||
| await tick(); | ||
| _actionsSlotHasContent = !!$$slots.actions; | ||
| function checkActionsSlotContent() { |
There was a problem hiding this comment.
This function just needs to be moved down after the reactive $: blocks..we can leave it for now.
|
|
||
| // Hysteresis threshold: prevents jitter when dynamic edge margin/height | ||
| // changes shift the scroll position slightly at boundaries. | ||
| const SCROLL_HYSTERESIS = 20; |
There was a problem hiding this comment.
Having a component level constant that is only used within one function makes more sense to just move this constant into the function, otherwise I am looking around for where else it is used, as with the other comment this can be left as is for now.
There was a problem hiding this comment.
ah ok, good to know. Thanks.
V2 visual updates for PushDrawerInternal: - Heading-s (shrinks to heading-xs on scroll) - Flex-based header/footer pinning with scroll-aware borders and shadows - Floating inset margin using --goa-drawer-offset token - Border color greyscale-150 - Dynamic edge transitions: visible edges get offset + rounded corners, scrolled edges go flush + square. 200ms ease transitions. - 20px scroll hysteresis prevents jitter at boundaries - Version prop validated with typeValidator matching Drawer.svelte - V2 unit tests added (5 tests) - Empty actions detection via slot assignedElements - Conditional actions span wrapper in PushDrawer.svelte - V1 rendering completely unchanged Test page updated to app-shell layout matching real-world usage.
f18ff47 to
dd3d498
Compare
|
@chrisolsen conflicts resolved now, ready to merge |
|
🎉 This PR is included in version 1.41.0-dev.24 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.11.0-dev.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 6.11.0-dev.16 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.11.0-dev.17 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Closes #3580
Screen.Recording.2026-03-14.at.10.28.03.AM.mov
Test plan
npm run buildpassesnpm run test:prpasses