Add type bindings for overflow, overflow-x and overflow-y options

[jannesiera]

To get my feet wet regarding the strengthening of the type-safety of the CSS dsl.

I've added options for overflow and its cousins overflow-x and overflow-y, accoding to the spec. All comments on the options are directly copied from their description in the spec.

[MangelMaxime]

Hum, we are facing a problem here.

overflow: hidden visible is a valid CSS value.

The overflow property is specified as one or two keywords chosen from the list of values below. If two keywords are specified, the first applies to overflow-x and the second to overflow-y. Otherwise, both overflow-x and overflow-y are set to the same value.

[jannesiera]

Seems like I was being a bit too eager here :).

Would something like this work?

    static member Overflow (overflow: OverflowOptions, ?overflowY: OverflowOptions) =
        let values =
            match overflowY with
            | Some value -> [ overflow; value ]
            | None -> [ overflow ]
        unbox ("overflow", keyValueList CaseRules.LowerFirst values)

[MangelMaxime]

No, because it needs to return a string not an Object

This need to be tested

    static member Overflow (overflow: OverflowOptions, ?overflowY: OverflowOptions) =
        unbox ("overflow",  unbox overflow + " " + unbox overflowY)

We can unbox OverflowOptions values to string type because OverflowOptions is marked with StringEnum and so they indeed are string at runtime.

[jannesiera]

Right! I'll go ahead and test this one then.

[jannesiera]

I've tested these changes in a consumer project and they work.

Returning the unboxed value directly probably wasn't a good idea, and wouldn't work for the SSR code. So I ended up returning a CSSProp.Custom with the value.

[jannesiera]

I was too quick with my test and noticed that it actually didn't work when only passing a single argument (leaving out the optional value). Not unboxing the optional directly fixed that issue.

[jannesiera]

I believe these changes could be merged.

I'm planning to do more of these, and hopefully cover all the properties. It might be good to go step by step.

[MangelMaxime]

I am waiting @alfonsogarciacaro feedbacks.

Personally, I think if we accept changes like these we need to do all of them in one major release (can be stored in a separate branch and contribute into several PRs). Otherwise, each time someone upgrade Fable.React he will have breaking changes.

1. It's annoying to have breaking on each minor updates
2. Make it feels like unstable

[jannesiera]

Added DUs for various other CSSProps.

[alfonsogarciacaro]

Hmm, not sure if this will work on .NET side for SSR, but this makes me realize we may also have a problem with other StringEnum options. I need to look into that.

[alfonsogarciacaro]

I will merge this shortly and fix the SSR issue myself because this has made me realize we have an issue with the other CSS props.

However, as it's a breaking change in principle we should release it as a new major version. @MangelMaxime What's the situation with the react/react-dom split? Has something be decided for elmish/react?

[MangelMaxime]

However, as it's a breaking change in principle we should release it as a new major version.

I agree but only if all the options have been converted to the typed way otherwise it will feel hacky. If not, we can just release a fix for the SSR code.

What's the situation with the react/react-dom split? Has something be decided for elmish/react?

I need to take the time to do the split because it was a bit harder than planned :) You can ignore this situation, I will adapt the repos when ready. I am focusing on Nacara right now to solve several issues before starting another task.

[jannesiera]

However, as it's a breaking change in principle we should release it as a new major version.

I agree but only if all the options have been converted to the typed way otherwise it will feel hacky. If not, we can just release a fix for the SSR code.

I concur.

I'd really like to cover all the props. There's a lot of them so it's going to take a lot of time. I'm starting with the simpler ones that can be turned in a DU with a StringEnum annotation. Afterwards there will be a lot of them which will need a similar treatment as the overflow prop because they can receive optional parameters, etc.

I'll also do a proposal to treat pixels/percentages/length etc as a seperate type to have them type-safe and compliant with the css spec. A proposal is not ready yet, and I'd be happy to hear your opinions.

[MangelMaxime]

No worry @jannesiera my goal isn't to rush you or someone to do it for tomorrow :)

But, I wasn't comfortable with shipping current version of Fable.React with various style (some type safe and some obj) so I would like to no redo it.

Especially, when your PR and the code for fixing SSR are not linked together. :)

[alfonsogarciacaro]

I'm thinking about the issues with SSR: most of them happen because the special/hacky features of Fable like keyValueList or StringEnum (causing also situations like #174). These were ones of the few hacks surviving from Fable 1 to allow the compiler optimization that converts the props list to a JS object directly at compile time (when possible)

However, as this is causing many little issues when sharing the code with .NET maybe it's better to discard the hacks and use more standard code. This may bring a little runtime overhead but it shouldn't be noticeable. What do you think?

[alfonsogarciacaro]

About the units, it's a good idea. Though as we're using unit types, we would have to add extra cases (like HeightPx, HeightPercentage...) or mask static members as union cases (another hack).

Also, @Zaid-Ajaj Feliz project is already exploring this direction. Maybe we can wait for feedback from users of that library before making similar changes here.

[MangelMaxime]

This may bring a little runtime overhead but it shouldn't be noticeable.

Compare to today, this means that we will iterate over each list element one more time in order to check if we need to convert the Style element.

As a side note, if we change the Style representation, this has to be done as a major version because it can break existing library I think.

Is SSR really broken? Because I am surprised that no one complained yet about it.

I think, we should move the SSR discussion to a separate issue.

[alfonsogarciacaro]

TBH, I don't know. I don't use SSR myself and probably not many people use it yet. If there's a problem, it's probably a few CSS rules not working correctly and they may be overriden by React on the frontend. That's maybe the reason nobody has noticed it yet.

[alfonsogarciacaro]

Ok, I'll merge this PR so I don't forget and I'll try to see what can I do about the SSR thing :)