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

Check boolean attributes presence, not value #71

Closed
carson-katri opened this issue Nov 8, 2023 · 6 comments
Closed

Check boolean attributes presence, not value #71

carson-katri opened this issue Nov 8, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request JETPACK Label for JetPack

Comments

@carson-katri
Copy link

carson-katri commented Nov 8, 2023

Currently, the client is checking if boolean attributes (like selected) are equal to the string "true". However, boolean attributes in HTML are intended to be read by presence. If the attribute is on the element it's true, even if the value is the string "false". If the attribute is not present, the value is false.

<!-- all of these would be selected -->
<RadioButton selected />
<RadioButton selected="true" />
<RadioButton selected="false" />

<!-- this form would be deselected -->
<RadioButton />

With this approach, users can pass bools in without encoding them as strings. LiveView should automatically add/remove the attribute from the template.

<RadioButton selected={@is_selected} />

<!-- when `@is_selected` is `true` -->
<RadioButton selected />
<!-- when `@is_selected` is `false` -->
<RadioButton />

The HTML Specification makes this note:

The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

@bcardarella
Copy link
Contributor

bcardarella commented Nov 13, 2023

I spoke to @nelson-glauber about this, here is my understanding:

There are certain Jetpack components that have attributes with default values of true. In this case when the attribute is omitted from the markup we should respect the default. However, in HTML an omitted attribute is assumed to be false. LiveView is repsecting this as demonstrated in @carson-katri comment:

<RadioButton selected={false}>

will render

<RadioButton>

but because Jetpack has a default value for selected being true we have a misalignment. What we instead want is to not strip out the attribute from the markup in the event an attribute is set to a falsey value.

<RadioButton selected={false}>

this should instead render

<RadioButton selected=false>

Currently @nelson-glauber is working around this by using a stringified value of the boolean:

<RadioButton selected={Atom.to_string(false)}>

this will correctly render:

<RadioButton selected="false">

and @nelson-glauber is typecasting this back to false boolean inside the client. This works but not ideal. I'm investigating if this is something we can change without an upstream request to Phoenix or not.

@nelson-glauber nelson-glauber added the JETPACK Label for JetPack label Nov 14, 2023
@carson-katri
Copy link
Author

I personally think it's clearer if you just swap the default on the client.

If you always pass selected to the component as false if not present, and true if present, then the default in Jetpack Compose wouldn't matter. Users would have to pass any boolean attributes they want to be true, even if the default would normally be true.

@nelson-glauber Do you have an example of a component that has an argument with a default value of true?

@bcardarella
Copy link
Contributor

@carson-katri if there are similar compromises being made in the SwiftUI client that would be against our stated goal of achieving as close to feature parity with the target framework as possible. We really need to divorce our thinking away from HTML and the limits and specification there and think of this as a markup representation of the target UI framework. We have no influence over the upstream framework's API and need to conform to those expectations and not deviate for the convenience of what works in HTML.

@carson-katri
Copy link
Author

I found one instance of a Bool attribute with a default value of true in the SwiftUI client.

See ScrollView.showsIndicators

@nelson-glauber
Copy link
Collaborator

@bcardarella
Copy link
Contributor

Closed by liveview-native/live_view_native#138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request JETPACK Label for JetPack
Projects
Status: Done
Development

No branches or pull requests

3 participants