Reactive properties and styles - #60
Conversation
Preparing to add other kv: styles and props
Will come in handy when adding new key-values
fdb2c9d to
ac76a88
Compare
voodoos
left a comment
voodoos
left a comment
There was a problem hiding this comment.
Did a first round of review, I have a good overall feeling about it.
I do like the use of 'a El.Prop.t which makes the api cleaner, but it is a shame that it requires wrapping... We are already putting P, R, S all other the place... P (P ...) is a bit confusing, `P (Prop ...) a bit verbose. Not sure what's the best approach here.
| @@ -26,11 +33,23 @@ val v : ?d:document -> ?at:At.t col -> ?ev:handler col -> tag_name -> t col -> t | |||
There was a problem hiding this comment.
Maybe you do that in another commit, but we should document the new parameters, and maybe make the "same attrib more than once" behavior clearer. Also there is at least one typo we could fix: occurences, (Claude, where are thou !?)
There was a problem hiding this comment.
Who is this Claude everyone is talking about?
There was a problem hiding this comment.
I went with something but I could not find any docstring that would make the "same attrib more than once" behavior clearer...
| - Lwd.set: change binding before invalidation, otherwise the old value could be re-observed (reported by @voodoos) | ||
| - brr-lwd: support declarative events and set of css classes | ||
| - Nottui: fix treatment of some terminal events that were delayed because of improper buffer flushing, reported by @darrenldl | ||
| - brr-lwd: Fix handling of multiple reactive attributes (by @panglesd) |
| El.set_prop k v el | ||
| in | ||
| let dummy_kv = (Jstr.empty, Jv.undefined) in | ||
| let dummy_kv = P (El.Prop.width, 0) in |
There was a problem hiding this comment.
Yes :( I don't like this dummy_value too much, but it is safe.
I did not do the effort to get rid of it, but please do if you prefer!
Not sure too. Also, Probably, the "less clean" api is the more pragmatic way to go... But I'm happy to be convinced otherwise (I tried it on slipshow, it was okayish but not much better. |
2 participants
Add reactive properties and styles.
Does not address #59, but reactive styles are unset when "not present anymore", while reactive props are not (as it would not make sense.
One of the commit is optional, it makes the API in some ways better (uses
Brr.El.Prop.t) and worse (it requires a constructor to hide an existential type.Brr.El.Prop.tis restricted to basic types, but I did not find an example where an object type is needed). What do you think?I also added two examples.
I'm not sure the
dummy_valueis a good OCaml pattern, but I kept it anyway! One step at a time!It's probably good to review commit by commit. I tried to have a clean history. It starts with some refactoring to add abstractions to make adding styles and props easier. Then, styles, then props are added.
Also, notes that for the reason given in #54 (comment), I kept the distinction between props and attributes.
Ping @voodoos!