search

ocsigen / ocsigen-toolkit

User interface widgets for OCaml applications
Other
32 stars 20 forks source link

[ot-color-picker] Explicitly return the entire signal #205

Closed rodibozman closed 3 years ago

rodibozman commented 3 years ago

…to set the signal in addition to its value.

In the function make of the file ot_color_picker.eliom (also see the .eliomi) we return the entire cp_sig and not just the fst attribute of the cp_sig in order to be able to re-set it if we want to.

(I don't know if the reindent of the ocaml formatter is an issue but let me know I will reindent just like the previous way if needed)

rodibozman commented 3 years ago

@clembu Would something like my last commit be better ?

balat commented 3 years ago

Other interface to test: 

make : ?update:(... React.E.t) -> ... -> ... React.S.t

Try to keep the interface similar to other widgets. May be return a record?

rodibozman commented 3 years ago

New interface is available (from last commit) Of all the interface I tried, this one is the most uniform with the rest of the toolkits already existant. and works really fine (I tried it on the graffiti tutorial).

They're was also a problem that we didn't even notice at first but is crucial in order to be able to use the color picker correctly, in the signature at first it was something like :

val make : ?a: [< Html_types.div_attrib > `Class ] Eliom_content.Html.attrib list ->
           [> `Div ] Eliom_content.Html.D.elt * (int * float * float) Eliom_shared.React.S.t

when it should have been : 

val make : ?a: [< Html_types.div_attrib > `Class ] Eliom_content.Html.attrib list -> unit ->
           [> `Div ] Eliom_content.Html.D.elt * (int * float * float) Eliom_shared.React.S.t

Indeed they're was the unit type that was missing, without it, you can not call make properly without giving the a attribut.

That is why I added the unit type in the signature of make in the last commit as well.

rodibozman commented 3 years ago

I have made a little bit of cleaning in the commits, the PR is ready to be merged !

balat commented 3 years ago

I think the ocamlformat config file is missing

rodibozman commented 3 years ago

As you suggest it to me @balat, I have made cleaning in the history, I added the ocamlformat config file, and I reindented (once and for all) all the files with thoses commands : 

> ocamlformat -i src/widgets/*.eliom
> ocamlformat -i src/widgets/*.eliomi

Now we can all work with the same "save format" with all the current files.

rodibozman commented 3 years ago

@balat Changes are made after our conversation, the PR is ready to be merged.

rodibozman commented 3 years ago

@balat Correction for the few mistakes are done, the modifications are at the correct commit now 👍🏼