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

Add upgrade() whenDefined() to CustomElementRegistry #831

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cyz1901
Copy link

@cyz1901 cyz1901 commented Jan 14, 2024

Hi there!

Before submitting a PR containing any Scala changes, please make sure you...

  • run sbt prePR
  • commit changes to api-reports

Thanks for contributing!

add upgrade() whenDefined() to CustomElementRegistry

@cyz1901 cyz1901 changed the title add upgrade whenDefined to CustomElementRegistry Add upgrade whenDefined to CustomElementRegistry Jan 14, 2024
@cyz1901
Copy link
Author

cyz1901 commented Jan 14, 2024

Hello team. I am not sure should we also add the get() and getName() functions, which seems to involve the type being a constructor...

@cyz1901 cyz1901 changed the title Add upgrade whenDefined to CustomElementRegistry Add upgrade() whenDefined() to CustomElementRegistry Jan 14, 2024
@armanbilge
Copy link
Member

Hmm. I think the type for a constructor should be js.Dynamic. For example see val constructor in this API:
https://www.scala-js.org/api/scalajs-library/latest/scala/scalajs/js/ConstructorTag.html

@cyz1901
Copy link
Author

cyz1901 commented Jan 14, 2024

Hmm. I think the type for a constructor should be js.Dynamic. For example see val constructor in this API: https://www.scala-js.org/api/scalajs-library/latest/scala/scalajs/js/ConstructorTag.html

Added

Copy link
Contributor

@zetashift zetashift left a comment

Choose a reason for hiding this comment

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

Could use some doc comments, but otherwise looks good to me! (You might have to format my example of doc comments).

cyz1901 and others added 3 commits March 6, 2024 23:50
@cyz1901 cyz1901 requested a review from zetashift March 6, 2024 16:58
@cyz1901
Copy link
Author

cyz1901 commented Mar 6, 2024

fixed

Copy link
Contributor

@zetashift zetashift left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -11,11 +11,29 @@ import scala.scalajs.js.annotation._

/** The CustomElementRegistry interface provides methods for registering custom elements and querying registered
* elements. To get an instance of it, use the window.customElements property.
* https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry

*/
@js.native
@JSGlobal
abstract class CustomElementRegistry extends js.Object {

/** Returns the constructor for a previously-defined custom element. */
def get(name: String): js.Dynamic
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get(name: String): js.Dynamic
def get(name: String): js.UndefOr[js.Dynamic]

/** Returns an empty Promise that resolves when a custom element becomes defined with the given name. If such a custom
* element is already defined, the returned promise is immediately fulfilled.
*/
def whenDefined(name: String): js.Promise[Any]
Copy link
Member

@armanbilge armanbilge Mar 20, 2024

Choose a reason for hiding this comment

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

Please copy the doc from here.

Suggested change
def whenDefined(name: String): js.Promise[Any]
def whenDefined(name: String): js.Promise[js.Dynamic]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants