GenieFramework / StippleUI.jl

StippleUI is a library of reactive UI elements for Stipple.jl.
MIT License
83 stars 15 forks source link

Radiobutton does not update reactive field for val = "false" #57

Closed jochenkrattenmacher closed 1 year ago

jochenkrattenmacher commented 2 years ago

Hello! In the example below, the radiobutton with val = "true" works just fine (as expected), however, with val = "false" it doesn't work. For some reason, val = 0 works as well (which works as a workaround of some sorts). To be a bit more precise, clicking on the radiobutton with val = "false" doesn't trigger on(model.show_bar)

using Stipple, StippleUI, StippleCharts

using DataFrames

data = DataFrame(Costs = [44, 55, 13, 43, 22])
labels = ["Team A", "Team B", "Team C", "Team D", "Team E"]
chart_width = 500
barchart = [PlotSeries("name", PlotData(data.Costs))]
piechart = data.Costs
plot_options_bar = PlotOptions(;
    chart_type = :bar,
    chart_width,
    labels,
)

plot_options_pie = PlotOptions(;
    chart_type = :pie,
    chart_width,
    chart_animations_enabled = true,
    stroke_show = false,
    labels,
)

@reactive! mutable struct Example <: ReactiveModel
    plot_options::R{PlotOptions} = plot_options_pie
    chart::R{Union{Vector,Vector{PlotSeries}}} = piechart
    show_bar::R{Bool} = false
end

Stipple.register_components(Example, StippleCharts.COMPONENTS)
model = Stipple.init(Example)

function switch_plots(model)
    println("hello")
    if model.show_bar[]
        model.plot_options[] = plot_options_bar
        model.chart[] = barchart
    else
        model.plot_options[] = plot_options_pie
        model.chart[] = piechart
    end
    return model
end

function handlers(model)
    on(model.isready) do ready
        ready || return
        notify(model.plot_options)        
    end
    on(model.show_bar) do _
        switch_plots(model)     
    end
    model
end

function ui(model)
    dashboard(model, [
        StippleUI.Layouts.layout([
            header([
                    btn("",icon="menu", @click("drawer = ! drawer"))
                    title("Example App")
            ])
            h3("Example Plot")
            row(
                StippleCharts.plot(:chart, options = :plot_options)
            )
            row(
                [radio("Pie plot", :show_bar, val = "false"), #does not work, but works if "val = 0" instead
                radio("Bar plot", :show_bar, val = "true")]
            )
        ])
    ])
end

route("/") do
    model |> handlers |> ui |> html
end

up(8080, open_browser = true)
hhaensel commented 2 years ago

Leave out the quotes: val = true

jochenkrattenmacher commented 2 years ago

Leave out the quotes: val = true

thanks for the fast response! Unfortunately, if I do that, neither of the radiobuttons work for me

hhaensel commented 2 years ago

Oh, I remember. That's a weird issue that comes from earlier times. We used to convert "false" to false. Not sure whether we corrected this on master already. I'll check.

hhaensel commented 2 years ago

Oh, I remember. That's a weird issue that comes from earlier times. Currently we convert "false" to false in Genie. https://github.com/GenieFramework/Genie.jl/blob/8c3788a773e5d9ca9c63531e7e498b7d534d980e/src/renderers/Html.jl#L248 @essenciary Didn't we want to change this?

hhaensel commented 2 years ago

You could get around the issue by adding a space character I guess. So please try "false "

essenciary commented 2 years ago

@hhaensel I did change it in the Genie #v5 branch, but as I recently mentioned it still doesn't work correctly. We basically need to support two situations: 1/ boolean html attributes, such as checked or selected. According to the HTML spec (and the way it's implemented in the browsers), for a false value, the attribute must be absent. This means that things like checked = "false" are actually evaluated as checked being true. https://html.spec.whatwg.org/#boolean-attribute 2/ Other attributes where we explicitly want to output the value false.

At the moment Genie forces scenario 1 -- and it's difficult to support both since they are effectively incompatible.

We could do a few things: a) only force the boolean false specification of removing the attribute for the 20+ HTML boolean attributes from the spec. This is pretty safe but I don't know what happens if other web components will expect their custom boolean attributes to behave in the same way. b) provide a failsafe to enable/disable this behaviour (like always considering False as a boolean attribute). Issue is that people will not know about it and/or might unintentionally do it.

But I think implementing these 2 is a good compromise.

hhaensel commented 2 years ago

Now my two cents on this issue (... and a correction of some of my proposals that I didn't test and that did not work ...)

FIrst of all a link to the HTML spec with nice examples for a common understanding. This leads to the following behavior of our API:

julia> radio("Bar plot", :show_bar, val = true)
"<q-radio val label=\"Bar plot\" v-model=\"show_bar\"></q-radio>"

julia> radio("Bar plot", :show_bar, val = false)
"<q-radio label=\"Bar plot\" v-model=\"show_bar\"></q-radio>"

There are several ways out of the problem:

julia> radio("Bar plot", :show_bar, val = R"false")

in Genie 4

"<q-radio label=\"Bar plot\" v-model=\"show_bar\">"

in Genie 5

"<q-radio :val = "false" label=\"Bar plot\" v-model=\"show_bar\">"

hhaensel commented 2 years ago

NOTE: If you update to the latest Stipple versions you need to use ]add StippleCharts#master as the latest changes in Stipple were not yet applied in StippleCharts. I just pushed the necessary modifications. Also change dashboard to page

hhaensel commented 2 years ago

@essenciary shall I tag a new version of StippleCharts?

essenciary commented 2 years ago

@hhaensel 1/ I'm not sure about using :value instead of value - the first one is a reference to Vue prop, the second is an "inline" value. The initial question was about using value. 2/ tag - sure. Though FYI, we're not officially supporting StippleCharts anymore as we don't have bandwidth for maintaining two plotting libraries. It has been deprecated in favour of StipplePlotly.

hhaensel commented 2 years ago

@hhaensel 1/ I'm not sure about using :value instead of value - the first one is a reference to Vue prop, the second is an "inline" value. The initial question was about using value.

:val is not necessarily a Vue property, it can be any js expression. It can contain Vue propoerties. We use this fact when we link PlotlyBase Charts with plot("plot.data", layout = "plot.layout") or when we reference array elements , e.g. color = R"colors[0]"

julia> plot("plot.data", layout = "plot.layout")
"<plotly :data=\"plot.data\" :layout=\"plot.layout\"></plotly>"

julia> btn("", @click(:action), color = R"colors[0]")
"<q-btn label :color=\"colors[0]\" v-on:click='action = true'></q-btn>"

EDIT: It's also the only way of assigning more complicated values like dicts, which I used for the setting static layout and config in StipplePlotly:

julia> xelem(:hh, mydict = Symbol(StipplePlotly.Charts.jsonrender(Dict(:a => 1, :b => Dict(:c => "Hello", :d => "World")))))
"<hh :mydict=\"{'a':1,'b':{'d':'World','c':'Hello'}}\"></hh>"
essenciary commented 2 years ago

Ah yes, good point.

hhaensel commented 2 years ago

So can we close this?

essenciary commented 2 years ago

It's addressed on the Genie 5 branches - only HTML bool attributes, per the HTML spec, remove the ="false" part, any other properties will keep it.

I'd say let's leave it open until it gets released and confirmed.

hhaensel commented 1 year ago

I added a fix which is in line with the latest changes to allow for Julia values in attributes and arguments.