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

Fix DateRange fields' names #62

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wildart
Copy link

@wildart wildart commented Jun 22, 2022

As the DatePicker element original comes from Quasar, where the date range specified by from & to model properties. We need to set the same names in the corresponding wrapper Julia type to harmonize names.

Fixes Stipple.jl/#117

@essenciary
Copy link
Member

Thanks, not sure about this, as a matter of principle the Julia implementation should not mimic the frontend names, especially if they are bad names (like in the case). The reason is, what if next month we decide to implement a non-quasar DateRange that is better, do we change the Stipple implementation in a breaking way to match that? The objective of StippleUI is to provide a Julian wrapper, abstracting away the JS layer, and improve where possible (and start and stop are really bad names IMO).

Can you please explain the background of this? I understand that in your situation it was simply a documentation issue? Why not contributing to improve the DateRange docs instead?

@wildart
Copy link
Author

wildart commented Jun 23, 2022

Can you please explain the background of this? I understand that in your situation it was simply a documentation issue?

It isn't related to docs. DateRange type fields are rendered as object with from and to fields, as required for range mode. But, if you try to wire DateRange field to a different UI component, e.g. QInput, as follows:

@reactive mutable struct Model <: ReactiveModel
  daterange::R{DateRange} = DateRange()
end

textfield("", Symbol("daterange.start"))

The textfield cannot access an actual filed name of DateRange.start. I guess because it was renamed to from on JS side, but textfield component is able to read field start field of DateRange object if the JS name is provided:

textfield("", Symbol("daterange.from"))

It looks like there need to be some feedback when the render Julia type rendered differently to JS, or the name should be synchronized on both Julia & JS side.

@essenciary
Copy link
Member

Thanks - ah I see, you're looking to access the JS side of the property. OK, in this case it's a powerful reason to keep the same names.

@essenciary
Copy link
Member

essenciary commented Jun 24, 2022

The issue is though that start and stop are the Julia properties names for the StepRange.

julia> dr = StepRange{Date,Day}
StepRange{Date, Day}

julia> fieldnames(dr)
(:start, :step, :stop)

@wildart
Copy link
Author

wildart commented Jun 29, 2022

Why do you need specific names for the range type? From Julia side, any type can be itertateble if iterate function is implemented for this type, but on JS side the names would be consistent with the framework element.

@essenciary
Copy link
Member

essenciary commented Jun 30, 2022

The overarching principle is to have a Julia implementation where the frontend is abstracted away and invisible. So if a struct is called a Range it should work like, or be, a Range. And in fact, while we're at it, we should make the DateRange an actual Julia Range. So I would take it even further and make DateRange a subtype of OrdinalRange{Date, Day} because date ranges are supported natively in Julia.

julia> dr = Date(2014,1,29):Day(1):Date(2014,2,3)
Date("2014-01-29"):Day(1):Date("2014-02-03")

julia> typeof(dr)
StepRange{Date, Day}

julia> typeof(dr) |> supertype
OrdinalRange{Date, Day}

julia> typeof(dr) |> supertype |> supertype
AbstractRange{Date}

So while the issue you raise is valid, I don't think the proposed approach is the way to go - and nor is the change necessary.

For instance, this is how I implemented textfield + datepicker in a production app - there is no need to reference the JS props, we use data sync. By binding both elements to the same property filter_startdate, we leverage reactivity to keep them in sync.

          textfield("Start date", :filter_startdate, clearable = true, filled = true, [
            icon(name = "event", class = "cursor-pointer", style = "height: 100%;", [
              popup_proxy(cover = true, transitionshow = "scale", transitionhide = "scale", [
                datepicker(:filter_startdate, mask = "YYYY-MM-DD", navmaxyearmonth = "$(Dates.year(now()))/$(Dates.month(now()))")
              ])
            ])
          ])

Another way (which is something I have considered and would be useful for other use case too) is to someday offer an object API, in addition to the functional one. Ex now we have textfield and datepicker functions which directly output the code. We can maybe build code as TextField("Start date", :filter_startdate) > DatePicker() that provides specialized/optimized ways to build more complex UIs and automatically set up the bindings (generating something like the above snippet).

@wildart
Copy link
Author

wildart commented Jul 4, 2022

I reworked date range to use StepRange{Date,Day}. There is an addition conversion function needed in Stipple, which I'll submit in the separate PR.

Copy link
Member

@essenciary essenciary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be very helpful to include a minimum working example that we can use to set up a test.

@@ -133,17 +121,23 @@ function Base.parse(::Type{Date}, d::String) :: Date
end

function Stipple.render(dr::DateRange, _::Union{Symbol,Nothing} = nothing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work as you removed DateRange

@@ -133,17 +121,23 @@ function Base.parse(::Type{Date}, d::String) :: Date
end

function Stipple.render(dr::DateRange, _::Union{Symbol,Nothing} = nothing)
Dict(:from => dr.start, :to => dr.stop)
Dict(:from => dr.from, :to => dr.to)
end

function Stipple.render(vdr::Vector{DateRange}, _::Union{Symbol,Nothing} = nothing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, you removed the DateRange definition.

@essenciary
Copy link
Member

I reworked date range to use StepRange{Date,Day}. There is an addition conversion function needed in Stipple, which I'll submit in the separate PR.

Thanks, I have merged that into Stipple.jl - but this PR here does not look 100% right, I commented. An example/test would help to ensure all works as expected and so we can use as docs too.

@wildart
Copy link
Author

wildart commented Jul 7, 2022

The docstring for datepicker show a portion of the usage example.

I could also fix DatePickers example so it would work with this update.

@essenciary
Copy link
Member

Sorry, missed the updates - will review and follow back.

@hhaensel
Copy link
Member

hhaensel commented Nov 7, 2023

I think with the current version of datepicker() everything works fine.
@wildart Could you comment or close the issue?

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.

nested model properties
3 participants