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 Image facade and additional facades for HTMLImageElement #621 #725

Merged
merged 10 commits into from
Jan 27, 2023

Conversation

zetashift
Copy link
Contributor

Another good first issue!

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.

Overall LGTM! Thanks :)

The only thing I'm going think on is ImageLoadingMode. Unless we can find a more "official" name for it somewhere, "mode" seems fine to me.

@zetashift
Copy link
Contributor Author

Actually finding a name for ImageLoadingMode was the hardest part hahaha. I looked at the specs and it didn't have a name for it.
MDN also feels very unofficial with the "hint" so I used mode just to get started.
Policy sounds okay, but they also just drop that in a comment, the type itself doesn't really have a name...

I was also wondering if it should be Image and not HTMLImageLoadingMode :P. I don't have a real preference, hint could probably be the most familiar to people because MDN describes it like that.

@armanbilge
Copy link
Member

Ha yeah I thought as much 😂

Looking at the existing enums we have, both "mode" and "policy" are common. So I think either is a fine choice. No "hint".
https://github.com/scala-js/scala-js-dom/tree/main/dom/src/main/scala-3/org/scalajs/dom

Now it's just a philosophical question of what the difference between those is 🤔

@zetashift
Copy link
Contributor Author

I guess "mode" sounds like it can only be certain cases, whereas policy sounds more like "something that just should be followed but might not be"?

Naming is hard :P

@armanbilge
Copy link
Member

lol, well what about this? They seem to call it a "state"
https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes

The other thing that's notable is that it's not specific to Image, seems that iframe uses it too. So I wonder if it should actually be shared by both 🤔

@zetashift
Copy link
Contributor Author

Yuck, I don't find state fitting, but spec is spec...

Can rename to something like LoadingState

@armanbilge armanbilge linked an issue Nov 12, 2022 that may be closed by this pull request
@armanbilge
Copy link
Member

Ok ... coming back to this ... 😅

I think I'm in favor of LazyLoadingState. Since it's the "lazy loading attribute", and "Some attributes, called enumerated attributes, take on a finite set of states". So that seems like the specciest name.

@zetashift
Copy link
Contributor Author

@armanbilge I renamed it to LazyLoadingState and changed the Longs to Doubles!

@zetashift zetashift changed the title Add Image face and additional facades for HTMLImageElement #621 Add Image facade and additional facades for HTMLImageElement #621 Jan 23, 2023
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.

Great work on this, thanks for reminding me about it! 🚲 🏚️

@armanbilge armanbilge merged commit b2034ed into scala-js:main Jan 27, 2023
@zetashift zetashift deleted the add/#621 branch February 20, 2023 01:00
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.

Create facade for Image
2 participants