Closed AngelMunoz closed 2 years ago
This PR is more to propose something rather anything else, feel free to pick/discard stuff from here if you think there's a better way to do it
Thanks for this @AngelMunoz! Hmm, actually for the property settings I was thinking to use optional arguments in the Prop
constructor (sorry I didn't add an example of this in the initial draft). Using list of unions to represent options in JS is a pattern we started with Fable.React but has some problems (for example, nothing stops the dev from setting the same field more than once). Also, with the current implementation the props
object is automatically typed, but with this you need to type it by yourself and make sure the names and the types match those in the configuration, which is a bit of a pity because the compiler can do that work for you.
I was thinking to use optional arguments in the Prop constructor (sorry I didn't add an example of this in the initial draft)
That's great! I'm not sure why didn't that occurred to me before
Looks like you had a better approach in mind which sounds good I actually like the idea
@AngelMunoz I've added some property options (well, actually only one for now, "attribute") and I'm also requesting now a function for initialization. That way we don't need to create the props config object/styles argument on every render. It looks like this, let me know what you think: https://github.com/alfonsogarciacaro/Fable.Lit/blob/3c72ee385f5b6e3ed16b41940070c1f893fe7486/sample/src/Clock.fs#L96-L120
It'd be great if you could help to add the rest of the property options here.
I added the extra reactive properties but I'm concerned about these
state: bool
State is just an internal property that when changed it means the element knows it needs to re-render
reflect: bool
Reflect is needed to report any property changes back to the corresponding attribute (if it has one, i.e. attribute: true, attribute: "my-attribute"
)
converter: FromAttributeConverter
Converter works when we want to apply custom serialization/de-serialization and assign that to a property which may not be needed at all if we're just consuming the external values, perhaps one-way from attributes, but I think there might be performance concerns there
hasChanged: HasPropertyChanged
Custom detection for property changes (since JS objects don't have equality like F# ones tends to be useful there)
These are basically there to support the class based API from Lit plus the conversion between attributes and properties, besides converter (when used with a fromAttribute instead of both) If we're not adding setters to the Property I'm not sure if we should support them at all.
Thanks a lot!
I've made a few changes to make the arguments simpler and nicely typed, I hope it's ok. I've left out state
and noAccessor
because, if I'm not mistaken, these are meant for properties not exposed externally and used for internal state, a pattern we don't want to encourage.
Thanks to this PR property options are working very nicely! For example we can receive the list of color options from the attribute with a converter and also reflect the even-color
attribute when a color in even position is selected, which is detected by the css (see selector :host([even-color])
to set a border: https://github.com/alfonsogarciacaro/Fable.Lit/blob/7c300712253100cc00fbb281a9fb010fe8685ca8/sample/src/Clock.fs#L89-L111
I'm also experimenting with a useTransition
as I didn't find a nice way to do transitions with Lit. Seems to be working well, I guess it could also be adapted to React: https://github.com/alfonsogarciacaro/Fable.Lit/blob/7c300712253100cc00fbb281a9fb010fe8685ca8/sample/src/App.fs#L153-L187
If it's not too much to ask, it'd also be great if you could help with the tests 😸 I just set up a simple test project following Lit recommendations (note the tests are written in JS so we don't need bindings but the components are in F#).
Sure I can work on those, can you open an issue to track them? I'm guessing we should cover tests for all hooks, but I'm not sure what other parts
Following from https://github.com/alfonsogarciacaro/Fable.Lit/pull/7#issuecomment-916669138 I like the decorator approach, my mind still bends a little but I think I'm getting the hang of it.
I went ahead and tried to add part of the configurable parts of the properties to match Lit's API I'm still not sure how to make it as seamless as it was with your changes, but this also will allow authors to work with internal state properties or at least react to them when external sources set a property plus removing the limitation that attributes are strings only
Edit: kudos for the styles that go directly to the static property this will help with performance for project wide shared styles