-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Compose HTML: add an AttrsScope<*>.attr
extension function with a Boolean
parameter
#5178
base: master
Are you sure you want to change the base?
Conversation
* @see AttrsScope.attr | ||
*/ | ||
fun AttrsScope<*>.attr(attr: String, value: Boolean = true) = | ||
attr(attr, value.toString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason making it an extension function?
What do you think about adding this function to the AttrsScope
itself?
Would you like to add a test for this new function in AttributesTests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for replying. Adding it as a member function looks good to me too. And sure I'd love to add some tests for this.
… `attr` in the `AttrsScope` interface as suggested by @eymar
Hi, I have finished the requested changes. However, I have noticed something else that the compose-multiplatform/html/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt Lines 197 to 206 in e9597e7
Is this the new recommended way that's not documented yet? Shall we update the KDocs and this added boolean |
@ShreckYe , Good question!
So passing "false" doesn't really make it "false". And the current implementation you made won't be correct. Also here: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes#boolean_attributes
|
Thanks for replying and the references you provide. It explains everything.
For this, what I am thinking is just removing the |
The
AttrsScope.attr
member function supports bolean attributes via "For boolean attributes cast boolean value to String and pass it as value.". This feature turns out to be frequently used in this compose-html-material project I have been working on. So I'd like to propose to add this to the core library.