Skip to content

Feat: add loading dom prop #45

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

Merged
merged 2 commits into from
May 31, 2022

Conversation

LukasDeco
Copy link
Contributor

@LukasDeco LukasDeco commented May 13, 2022

I would like to add the loading property for Domprops. This allows someone to add the loading property on an image for using html-based browser-native lazy loading.

src/ReactDOM.res Outdated
@@ -291,6 +291,8 @@ module Props = {
@optional
list: string,
@optional
loading: string, /* "lazy", "eager" */,

Choose a reason for hiding this comment

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

We can specify the values exactly at the type level:

loading: [#lazy | #eager],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I made the change. I had to wrap lazy in quotes because it is a reserved keyword. Also, I used string because I noticed other props that are have a small/finite number of options just use string. (Like method and inputMode)

@Kingdutch
Copy link

This PR fixes #43 once the feedback from yawaramin is fixed :)

Copy link

@yawaramin yawaramin left a comment

Choose a reason for hiding this comment

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

Thanks. The other props use string type because they predate ReScript's feature of polymorphic variants compiling to strings. I think it would make sense to update the prop types where appropriate.

@LukasDeco
Copy link
Contributor Author

Is that something we want to do on this PR?

@yawaramin
Copy link

Not necessarily. I +1'd this PR as I don't think it needs to do anything else. My comment was more about future improvements.

@LukasDeco
Copy link
Contributor Author

@Kingdutch @yawaramin sorry for the delay. I can't seem to merge this PR though.

@cristianoc cristianoc merged commit ef32529 into rescript-lang:master May 31, 2022
@mununki
Copy link
Member

mununki commented Sep 1, 2022

Hi, I'm quite late for the party.
I'm wondering why the loading attribute is added into type domProps only, not into type props.

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.

5 participants