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

Remove unnecessary parameter to required #1988

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

veyndan
Copy link
Contributor

@veyndan veyndan commented Apr 2, 2022

As required is a boolean attribute, required(false) would confusingly make the input required.

As [`required` is a boolean attribute](https://html.spec.whatwg.org/multipage/input.html#the-required-attribute), `required(false)` would confusingly make the input required.
@veyndan veyndan force-pushed the veyndan.2022-04-02.required branch from 0a2a80f to 3b59b9a Compare April 2, 2022 17:46
@eymar
Copy link
Member

eymar commented Apr 4, 2022

@veyndan Thank you for your contribution.

Would you like to make some adjustments to ensure more smooth migration?

Index: web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt
--- a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt	(revision a027149b9527838798f8b590d7d9fa539b151473)
+++ b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt	(date 1649063275433)
@@ -194,8 +194,18 @@
 fun AttrsScope<HTMLInputElement>.readOnly() =
     attr("readonly", "")
 
-fun AttrsScope<HTMLInputElement>.required(value: Boolean = true) =
-    attr("required", value.toString())
+@Deprecated(
+    message = "Please use `required()` without parameters. Use if..else.. if conditional behaviour required.",
+    replaceWith = ReplaceWith("required()", "org.jetbrains.compose.web.attributes.required"),
+    level = DeprecationLevel.WARNING
+)
+fun AttrsScope<HTMLInputElement>.required(value: Boolean = true) : AttrsScope<HTMLInputElement> {
+    if (value) attr("required", "")
+    return this
+}
+
+fun AttrsScope<HTMLInputElement>.required() =
+    attr("required", "")
 
 fun AttrsScope<HTMLInputElement>.size(value: Int) =
     attr("size", value.toString())


 

The patch does:

  • Fixes the behaviour of existing requried(value: Boolean = true) method
  • Creates new method required() w/o parameters
  • Deprecates existing method (even if it's fixed, it's better to remove it later to have consistent methods). It would help users to understand what needs to be done in their codebases.

@eymar eymar self-requested a review April 4, 2022 09:14
@veyndan
Copy link
Contributor Author

veyndan commented Apr 9, 2022

I like the idea of deprecating the existing function and adding a new no-arg version. However, I don't think there is necessarily anything to "fix" with the existing function, as it does what it's supposed to do (which is just setting required=$value), even if that's surprising (as it is to me in regular html).

@hfhbd
Copy link
Contributor

hfhbd commented Sep 8, 2022

@eymar web too :)

@eymar eymar added the web label Sep 9, 2022
@eymar
Copy link
Member

eymar commented Sep 9, 2022

thank you!

@eymar eymar merged commit dd857bd into JetBrains:master Sep 9, 2022
@veyndan veyndan deleted the veyndan.2022-04-02.required branch February 4, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants