Skip to content
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

Refactor the drawer component #167

Closed
endigo9740 opened this issue Aug 28, 2022 · 11 comments
Closed

Refactor the drawer component #167

endigo9740 opened this issue Aug 28, 2022 · 11 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request ready to test Ready to be tested for quality assurance.
Milestone

Comments

@endigo9740
Copy link
Contributor

endigo9740 commented Aug 28, 2022

The Drawer component suffers from a number of issues:

  • The documentation is a mess and doesn't properly detail how to implement or use this component
  • We should probably designate it a Utility, as it's not really a standard component
  • It needs to be more turnkey to implement, perhaps paired with App Shell
  • The width of the drawer should be customizable
  • I was having trouble implementing the visibility toggle
  • Should have an option to make it take fixed size or overlay on demand (not just responsively)
@endigo9740 endigo9740 self-assigned this Aug 28, 2022
@endigo9740 endigo9740 added documentation Improvements or additions to documentation enhancement New feature or request bug Something isn't working labels Aug 28, 2022
@endigo9740
Copy link
Contributor Author

This will come as part of this update:
#158

@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 31, 2022

Great progress on the Drawer today.

  • I've decoupled the AppShell left sidebar from the Drawer component, allowing for unique styling customization
  • This can now be used for ANY kind of slide out panel, not just hardcoded left sidenav
  • Both AppShell/Drawer now use a 'shared' component to create the list of links for the Docs site
  • Code is cleaner and more style props are possible
  • I've adjusted the theme styling to better match the Dialog backdrop (surface color rather than faded black)
  • It's now using Svelte's built in Transition methods (backdrop fades, drawer slides in)
  • Drawer component is now simpler (one slot), but comes with 4 positions (shown below)
  • Note only the left animation is working as expected so far, but I'll fix that.

Positon settings are set via position="left|right|top|bottom":

Screen Shot 2022-08-30 at 9 34 15 PM

@endigo9740 endigo9740 added this to the v1.0 milestone Aug 31, 2022
@endigo9740
Copy link
Contributor Author

Progress on the new Drawer component:
https://i.imgur.com/pyLQICQ.mp4

It now supports animation from any of the four positions. Works really well in execution!

@endigo9740
Copy link
Contributor Author

Vitest integration cases have been added. I think with this the component is ready for QA!

Please pull branch feat/app-shell to begin testing asap @thomasbjespersen @niktek

@endigo9740 endigo9740 added the ready to test Ready to be tested for quality assurance. label Aug 31, 2022
@endigo9740
Copy link
Contributor Author

endigo9740 commented Sep 5, 2022

When testing the Drawer component within the Skeleton docs project, the Svelte transition animations work as expected. The drawer shim fades in, and the drawer itself slides in from its respective position side.

https://i.imgur.com/8bviYW3.mp4

However, when bundled into a tarball (.tgz) package and installed in a test SvelteKit project, only the fade animation is functional. The fly/slide animation is not present:

https://i.imgur.com/FaPPG65.gif

We'll need to troubleshoot where and how this is failing. My guess is it's not picking u the window size used for the x/y transition settings.

@endigo9740
Copy link
Contributor Author

endigo9740 commented Sep 6, 2022

@niktek @LukeHagar

I'm at a bit of a loss on this issue. The Svelte fly transition works as expected when running the local dev server via npm run dev of the Skeleton project. It also works fine when you build npm run build and then preview with npm run preview. However, when I package and install in my standalone test app the fly animation always fails.

So far I've tried:

  • Running as is, current with the dev branch
  • Running with a different means of capturing the window width data
  • Running with hardcoded values for the fly transition x/y values in the template
  • Making the backdrop/drawer elements siblings rather than parent/child

Nothing seems to work. The fade animation always works on the backdrop, but the fly animation never does. Perhaps we've run into some kind of bug?

I'd welcome your help in testing this. This only thing I can think to do at this point is drop the Svelte transition and animate it directly. Though this will come with a much more significant overhead since there's animations in/out for four separate positions.

@endigo9740
Copy link
Contributor Author

Ok it gets a bit weirder still. If I drop the backdrop completely, make the drawer the root element in the component, then the fly animation works as expected! I'd been keen to use this, but it means the background area stays full opacity and you cannot click "off" the drawer to close it:

skeleton-drawer-no-backdrop

I thought perhaps having two animations within the same series of elements might be the the issue. So I attempted to make the drawer/backdrops siblings (both at the component template root). However, still no luck here. The backdrop fade seems to work, but the drawer fly fails. The drawer just pops into place:

skeleton-drawer-siblings

@endigo9740
Copy link
Contributor Author

endigo9740 commented Sep 6, 2022

Finally made some headway on this!

It appears the update from svelte@3.49.0 -> svelte@3.50.0 introduced some regressions or breaking issues for transitions. I've gone ahead and filed a ticket for our issue here:

sveltejs/svelte#7842

For now we should make the recommendation that folks pin to svelte@3.49.0 if they make use of the Drawer. Otherwise they should wait until an update drops that resolves this issue.

With this in mind we won't delay the release tomorrow for this issue. Please continue QA, but otherwise we'll expect this to go out early tomorrow morning my time!

@niktek
Copy link
Contributor

niktek commented Sep 7, 2022

Interestingly it works fine in Safari, but not Chrome/FF

@endigo9740
Copy link
Contributor Author

That's why cross-browser testing is important Svelte team! Heh

@endigo9740
Copy link
Contributor Author

Released as part of v0.37.32!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request ready to test Ready to be tested for quality assurance.
Projects
None yet
Development

No branches or pull requests

2 participants