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

Add a color picker widget #160

Closed
wants to merge 6 commits into from
Closed

Conversation

ilankri
Copy link
Member

@ilankri ilankri commented Apr 12, 2018

The code is imported from ocsigen-widgets; it was just tweaked (to try) to match the conventions of ocsigen-toolkit. As a side effect, these changes should normally close the issue ocsigen/ocsigen.github.io#10.

@vasilisp
Copy link
Contributor

There is a competing PR: #141. Can you take a look at the feedback for that one and possibly adjust your code?

@ilankri
Copy link
Member Author

ilankri commented Apr 12, 2018

Thank you @vasilisp. I saw it, but it seems that there were two kinds of color picker in ocsigen-widgets; one with a table and another one with hue and saturation selection. This pull request is for the latter and the one you mentioned is for the former.

@sagotch
Copy link
Contributor

sagotch commented Apr 12, 2018

I think that same feedback still apply to this PR. The way these widgets has been written in ocsigen-widget is really weird, especially regarding the rest of the code.

Code from oscigen-widget can not only be imported as is.

The strange style used in ocsigen-widget is a reason why ocsigen-toolkit has been created.

@sagotch
Copy link
Contributor

sagotch commented Apr 12, 2018

You would prefer a widget with signature such as:

unit -> [> `Div ] Eliom_content.Html.elt * (int * int * int) React.S.t

@sagotch
Copy link
Contributor

sagotch commented Apr 12, 2018

Also, the type and these funtion

val append_at : Dom_html.element Js.t -> t -> unit
val init_handler : t -> unit
val get_rgb : t -> int * int * int

are not needed

@ilankri
Copy link
Member Author

ilankri commented Apr 12, 2018

Thanks for the feedback, I will try to work on it.

@chrismamo1
Copy link
Contributor

@ilankri have you made much progress on this? I'm trying to update Graffiti for the tutorial, and it uses the OW color picker widget.

@ilankri
Copy link
Member Author

ilankri commented Apr 25, 2019

Not at all @chrismamo1, unfortunately, I have never had the opportunity to rework on it.

@chrismamo1
Copy link
Contributor

@ilankri Ok I'll try and pick up where you left off, and address the concerns brought up in this PR as well as the other one.

@jrochel
Copy link

jrochel commented May 21, 2019

superseded by: #175

@jrochel jrochel closed this May 21, 2019
@ilankri ilankri deleted the color-picker branch October 18, 2019 15:31
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.

5 participants