Webamp optionally fully contained into a DOM element#1338
Webamp optionally fully contained into a DOM element#1338captbaritone merged 4 commits intocaptbaritone:masterfrom
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.
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.
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! |
2 participants
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.