Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Façade Types to Support ResizeObserver #694

Merged
merged 11 commits into from
Apr 21, 2022

Conversation

chrisshafer
Copy link
Contributor

Mozilla Docs : https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver
Similar to MutationObserver

I wasn't sure about adding ResizeObserverEntry.devicePixelContentBoxSize, since as of now it isn't supported by Safari. I decided to leave out, but I would be happy to add it into this PR.

I also added DOMRectReadOnly, which seems a bit out of place in this PR but a few of the interfaces require that type. I'm also a bit concerned about adding it in this state due to the already implemented DOMRect. In the Mozilla docs it indicates that DOMRect extends the DOMRectReadOnly interface, but that doesn't really transfer well to Scala (vars vs defs). Looking for some guidance on approach here.

@armanbilge
Copy link
Member

armanbilge commented Apr 13, 2022

Thank you very much for the PR! To answer your questions:

I wasn't sure about adding ResizeObserverEntry.devicePixelContentBoxSize, since as of now it isn't supported by Safari. I decided to leave out, but I would be happy to add it into this PR.

Our current policy is to accept standardized/standards-track APIs, regardless of their browser support. So feel free to add it :)

https://github.com/scala-js/scala-js-dom/blob/main/CONTRIBUTING.md#facades

but that doesn't really transfer well to Scala (vars vs defs)

I think this is a question for @sjrd: could we do something like this and expect it to work, or something else?

trait FooReadOnly {
  def baz: Int = js.native
}

trait Foo extends FooReadOnly {
  def baz_=(x: Int): Unit = js.native
}

@sjrd
Copy link
Member

sjrd commented Apr 13, 2022

could we do something like this and expect it to work, or something else?

Yes, that works.

@chrisshafer
Copy link
Contributor Author

@armanbilge

    val rect = new DOMRect
    println(rect.left)
    rect.left = 10.0
    println(rect.left)
Cannot set property left of #<DOMRectReadOnly> which has only a getter

I'm assuming the _= + js.native doesn't work because there isn't a setter for the left field. Looks like there are only setters for x, y, width and height. This also breaks the current facade, as you get the same error. The setter works fine with height.

My guess is DomRect needs to loose the setters on left, right, bottom, and top, and we need to add x and y? Though thats a breaking API change.

In Chrome:

I tried mutating left with assignment in vanilla js as well which didn't work:

var rect = new DOMRect();

console.log("left");
console.log(rect.left);
rect.left = 10;
console.log(rect.left);

console.log("height");
console.log(rect.height);
rect.height = 10;
console.log(rect.height);
left
0
0
height
0
10
rect.x = 10
10
rect.left
10

Screen Shot 2022-04-14 at 10 30 47 AM

@armanbilge
Copy link
Member

Thanks for investigating!

Though thats a breaking API change.

Source-breaking, and that's fine consider it wasn't working anyway.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Took a first pass through with pointers about our current idioms/code style, thanks!

@chrisshafer chrisshafer force-pushed the cs/add-resize-observer branch from 6c4ca27 to d84ef7d Compare April 14, 2022 16:10
@chrisshafer
Copy link
Contributor Author

@armanbilge I think I addressed all your suggestions? Sorry I made a bit of a mess of the commits. I can squash when this is ready to go.

chrisshafer and others added 2 commits April 14, 2022 11:55
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Apologies, I did notice one last small thing :)

No need for a squash commit, personally I find keeping "messy" history to be valuable :)

Thanks for all of your work on this!

@chrisshafer
Copy link
Contributor Author

No problem at all. Happy to give back where I can 😄 . We use this library (and Scala.js as a whole) extensively since our platform/product is pure Scala. Appreciate what you all have done here!

@armanbilge armanbilge merged commit 4ff02dc into scala-js:main Apr 21, 2022
@chrisshafer chrisshafer deleted the cs/add-resize-observer branch April 24, 2022 16:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants