-
Notifications
You must be signed in to change notification settings - Fork 748
Webamp optionally fully contained into a DOM element #1338
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
❌ Deploy Preview for tourmaline-kringle-c98715 failed. Why did it fail? →
|
|
This is fantastic! Probably the number one most requested feature for Webamp. I'll try to find time for a more detailed review soon, but at a quick glance this looks broadly correct. A few thoughts:
Thanks for working on this! |
|
I think I already did both! :) |
|
packages/webamp/js/webampLazy.tsx
Outdated
| */ | ||
| async renderWhenReady(node: HTMLElement): Promise<void> { | ||
| this.store.dispatch(Actions.centerWindowsInContainer(node)); | ||
| async renderWhenReady(node: HTMLElement, contained?: false): Promise<void> { |
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 combined should eventually become the default behavior since the current behavior is confusing to most users. To enable that, what if instead of a flag, we added a new method: .renderInto(node) which had the new behavior? If you want you could make this configurable method as a private method which both renderWhenReady and renderInto call under the hood.
packages/webamp/js/webampLazy.tsx
Outdated
| }); | ||
|
|
||
| if (contained && getComputedStyle(node)?.position === "static") { | ||
| node.style.position = "relative"; |
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.
What if instead of silently modifying the DOM node we threw and error informing the user that they should avoid using position: static for the element into which Webamp is rendered?
|
Got so excited about this, I went ahead and added the suggestions myself so we can try to start trying this out. |
|
Great 👍 Thanks for this, I was on vacation in the last days and I didn't get the notifications from Github, just got back today. Happy new year! |
An optional boolean parameter in
renderWhenReadythat renders Webamp inside the DOM node passed as first argument, and makes the positioning restricted inside of it.I developed this by setting up a new Vite development server inside of
examples/contained: this can be deleted, but I figured out it could be useful to leave there for reference, as an additional example.