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

Color picker #141

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

Color picker #141

wants to merge 14 commits into from

Conversation

cedlemo
Copy link

@cedlemo cedlemo commented May 25, 2017

  • fix some typos,
  • rename some variables (lll_color*) and some functions
  • fix comments and improve description of functions/variables
  • add a CSS

I wait for your review.

Copy link
Contributor

@vasilisp vasilisp left a comment

Choose a reason for hiding this comment

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

This needs some work. Sorry for not being critical-enough on the code earlier. I know you didn't introduce the issues, but it would still be nice to bring the code up to a decent standard before merging.

let color_list = match precision with
| p when p <= 1 -> raise_color_samples_exception ()
| precision ->
let step = 255 / (precision - 1) in
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

done

let rec aux_red nl = function
| n when n >= max_iteration -> nl
| n ->
let red = List.nth color_list n in
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

done.


(* 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 =
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

done


(** Get the currently selected color of the selector. The fusion or add_square
functions have no effects on it. *)
val get_color: t -> string
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally use React.t signals for outputs like this.

Copy link
Author

Choose a reason for hiding this comment

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

@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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :).

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Attention to the plurals.

Copy link
Author

Choose a reason for hiding this comment

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

ok


(** 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
Copy link
Contributor

Choose a reason for hiding this comment

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

The color_div of the first color_picker

(** 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
Copy link
Contributor

Choose a reason for hiding this comment

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

keeps a reference to the first color_picker's block for appending a color in the future (?).

Copy link
Author

Choose a reason for hiding this comment

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

done


(** 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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, _) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can start happen automatically? I don't think we have similar start methods in the other Ocsigen Toolkit widgets.


(* Some pre-generated color samples in several precision. Color samples
are list of list of list of colors represented in string of hexadecimal
values.*)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

done

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.

2 participants