Color picker #141
Color picker #141
Conversation
| let color_list = match precision with | ||
| | p when p <= 1 -> raise_color_samples_exception () | ||
| | precision -> | ||
| let step = 255 / (precision - 1) in |
There was a problem hiding this comment.
step becomes 0 for precision >= 257, with undesired consequences. If we do input validation for precision <= 1, we had better do it for the upper bound as well.
Also, using match instead of if is not a good idea in this case. It makes the code less readable by introducing new identifiers.
src/widgets/ot_color_picker.eliom
Outdated
| let rec aux_red nl = function | ||
| | n when n >= max_iteration -> nl | ||
| | n -> | ||
| let red = List.nth color_list n in |
There was a problem hiding this comment.
That's very ugly code.
Instead of recurring on color_list, we recur on its indices and perform a linear-cost List.nth in every step! The List.nth inside aux_blue will increase the big-O complexity without reason.
Could you try to rewrite this in terms of nested List.map invocations?
|
|
||
| (* Take one list of list of list of color (string) and build table list with it. | ||
| return also div_color_list to allow to launch start script detection. *) | ||
| let generate_color_table color_samples = |
There was a problem hiding this comment.
Takes a list of lists of lists of colors (strings) and returns a table list. Also returns a div_color_list for launching start script detection.
The singular/plural needs to be fixed in the .eliomi.
|
|
||
| (** Get the currently selected color of the selector. The fusion or add_square | ||
| functions have no effects on it. *) | ||
| val get_color: t -> string |
There was a problem hiding this comment.
We generally use React.t signals for outputs like this.
There was a problem hiding this comment.
@vasilisp , sorry for the delay (I am an absolute beginner so I try to figure out how it works).
If I well understand, I should in fact modify the make function to make it returns something like that :
(t * div * div * string Eliom_shared.React.S.t)
where string Eliom_shared.React.S.t is a signal with the currently selected color like in the ot_time_picker ?
There was a problem hiding this comment.
Yes, that's what I have in mind. Maybe string option Eliom_shared.React.S.t, with the value of the signal being None before the user picks a color. Give it a try if you are likewise motivated :).
src/widgets/ot_color_picker.eliomi
Outdated
| val color_samples_6 : string list list list (* 1 table 2 columns 5 lines *) | ||
| val color_samples_10 : string list list list (* 1 table 2 columns 3 lines *) | ||
|
|
||
| (** Take one list (tables) of list (columns) of list (lines) of color (string) |
There was a problem hiding this comment.
Attention to the plurals.
src/widgets/ot_color_picker.eliomi
Outdated
|
|
||
| (** Get two color_picker to fusion in a single one. This new color_picker uses | ||
| color squares of both. | ||
| It uses color_div of first color_picker given in argument. It also keeps |
There was a problem hiding this comment.
The color_div of the first color_picker
src/widgets/ot_color_picker.eliomi
Outdated
| (** Get two color_picker to fusion in a single one. This new color_picker uses | ||
| color squares of both. | ||
| It uses color_div of first color_picker given in argument. It also keeps | ||
| referend on first color_picker's block to future append color |
There was a problem hiding this comment.
keeps a reference to the first color_picker's block for appending a color in the future (?).
|
|
||
| (** It allows to add square color after the start function. It have not to be | ||
| used before start *) | ||
| val add_square_color_and_start : t -> string list list list -> unit |
There was a problem hiding this comment.
I can't say I understand the logic of add_square_color*. Why do we have two of them? If the widgets has to be configurable with an extra component, can this happen via an optional argument to make?
| let fusion (color_ref, color_div, fst_list, block) (_, _, snd_list, _) = | ||
| (color_ref, color_div, fst_list@snd_list, block) | ||
|
|
||
| let start (color_ref, color_div, color_list, _) = |
There was a problem hiding this comment.
Can start happen automatically? I don't think we have similar start methods in the other Ocsigen Toolkit widgets.
src/widgets/ot_color_picker.eliomi
Outdated
|
|
||
| (* Some pre-generated color samples in several precision. Color samples | ||
| are list of list of list of colors represented in string of hexadecimal | ||
| values.*) |
There was a problem hiding this comment.
We need (** for this to appear in the doc.
You should probably leave a space before color_samples_p2 so that the comment refers to all the color_samples_p* that follow.
I wait for your review.