Skip to content

Conversation

@ansh7432
Copy link
Contributor

@ansh7432 ansh7432 commented Mar 4, 2025

Missing Support for the value Attribute

Description

This PR addresses Issue #428. The problem was that binding a signal to the defaultValue (or value) attribute of progress bars had no effect, preventing them from switching between determinate and indeterminate states as expected.

To resolve this, we have added a manual definition for the value attribute in our DSL—similar to the definition for the style attribute. This ensures that progress bars correctly update their state based on the underlying signal.

Reproduction

Below is a full reproduction snippet that demonstrates the issue (with no hacks):

import calico.IOWebApp
import calico.html.HtmlAttr
import calico.html.io.*
import calico.html.io.given
import cats.effect.IO
import cats.effect.kernel.Resource
import fs2.concurrent.SignallingRef
import fs2.dom.HtmlElement

object App extends IOWebApp {

  def render: Resource[IO, HtmlElement[IO]] = SignallingRef[IO].of(false).toResource.flatMap { ref =>
    div(
      progressTag(
        maxAttr := "100",
        value <-- ref.map {
          case true  => Some("20")
          case false => None
        }
      ),
      button("switch determinate/nondeterminate", onClick(ref.update(!_)))
    )
  }
}

When the signal toggles, the progress bar should switch between having a value (determinate state) and having no value (indeterminate state). However, due to missing support for the native value attribute, this behavior was not working as intended.

Changes Made

Manual value Attribute Definition:
Added a manual definition in the DSL (e.g., in calico/html/Html.scala) as follows:

def valueAttr: HtmlAttr[F, String] =
  HtmlAttr("value", encoders.identity)

This ensures that our DSL now has a dedicated valueAttr that correctly maps the value attribute to the native DOM property.

@ansh7432
Copy link
Contributor Author

ansh7432 commented Mar 5, 2025

Hii @armanbilge please review this getting error while compiling after adding value attribute

@armanbilge armanbilge self-requested a review March 6, 2025 04:44
Copy link
Owner

@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.

May I suggest an alternative approach? We can just add a value attribute as a one-off, similar to the style attribute:

def styleAttr: HtmlAttr[F, String] =
HtmlAttr("style", encoders.identity)

@ansh7432
Copy link
Contributor Author

ansh7432 commented Mar 6, 2025

Yeah thanks for the guidance applying the changes @armanbilge please re-review :)

@ansh7432 ansh7432 requested a review from armanbilge March 6, 2025 10:25
@kubukoz
Copy link
Collaborator

kubukoz commented Mar 6, 2025

I can try a snapshot of this but maybe we should have some sort of test with a "real" DOM, just as a regression testing thing

@armanbilge
Copy link
Owner

@ansh7432 thanks, can you update the PR description? 😁


but maybe we should have some sort of test with a "real" DOM

Yeah, Calico in general is low on tests ... what do you think is the precise thing that we should test? Really, the main things are does HtmlAttr actually manipulate attributes correctly, and that "value" is an attribute that exists.

@ansh7432
Copy link
Contributor Author

ansh7432 commented Mar 6, 2025

updated the pr descryption @armanbilge

@kubukoz
Copy link
Collaborator

kubukoz commented Mar 6, 2025

hmm in that case, maybe it's not needed.

Given it's the same code I tried in my hacks, LGTM

Copy link
Owner

@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.

thank you!

@armanbilge armanbilge merged commit 6859bd0 into armanbilge:main Mar 6, 2025
7 checks passed
@ansh7432
Copy link
Contributor Author

ansh7432 commented Mar 6, 2025

It's my pleasure @kubukoz @armanbilge willing to contribute more ☺️

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.

3 participants