Feature/Update Style API #215

akinsho · 2019-01-19T18:05:01Z

I was inspired by @wokalski suggestion in #205 idea of a change to the style api that would change it from

<Text
  style=Style.make(
     ~color=Colors.red
     ~backgroundColor=Colors.blue
    /* a bunch of stuff */
    (),
    )
  />

to

 <Text
  style=[
   fontSize(1)
   fontFamily(green)
   flexDirection(row)
   width(percent(90)) /* future plan that can come based on this */
  ]/>

Other examples of similar patterns are brisk, bs-nice and bs-css.
Current implementation so far is heavily inspired by brisk 😆

Advantages

The advantages of this would be a simpler, less error prone to newbie syntax (like forgetting to pass unit at the end of the make function) less exposure of the internal api of things like BoxShadow so this way it can be re-implemented and a user wouldn't have to refactor their usage of BoxShadow.make(~specificLabels)
Also merging styles would be easier as a list can be easily iterated and modified vs a record type

wokalski · 2019-01-19T18:21:09Z

src/UI/Style.re

+  | `Opacity(float)
+  | `Boxshadow(BoxShadow.properties)
+  | `Cursor(option(MouseCursors.t))
+];


This also gives you more room when it comes to accepting props. For instance it doesn't make sense for View to accept the `FontFamily prop!

rauanmayemir · 2019-01-19T19:17:38Z

src/UI/Style.re

+  | `Bottom(int)
+  | `Left(int)
+  | `Right(int)
+  | `FontFamily(string)


You might want to split up text style and 'position' props. Then you'll get to compose them like:

type buttonProps = [ textStyleProps | layoutStyleProps];

rauanmayemir · 2019-01-19T19:22:51Z

src/UI/Style.re

+let height = h => `Height(h);
+let width = w => `Width(w);
+let position = p => `Position(p);
+let margin = m => `Margin(m);


We managed to solve this like `Margin(inset) where type inset = {left: scalar, top: scalar, right: scalar, bottom: scalar}.

I got inspired by bs-css with its margin(m), margin2(~h, ~v), margin4(~l,~t,~r,~b) helpers, good thing flex has a handy cssUndefined value. (I'm not sure how safe it will be to use isUndefined on JS targets)

bryphe · 2019-01-20T16:58:20Z

examples/Border.re

-    (),
-  );
+  Style.[
+    backgroundColor(Colors.white),


Nice work @Akin909! I'm on board with this. Looks pretty concise and easy-to-understand, compared to the previous API! Really appreciate you putting together this proposal.

I also like that this would more easily support the style 'inheritance' that we really need for custom components. So that they can have a 'default' style but pieces of it can be overridden in a simple way.

examples/Border.re

bryphe · 2019-01-20T17:03:15Z

src/UI_Components/Button.re

-        <Text style={textStyle(~fontSize, ~fontFamily)} text=title />
+      <View
+        style=Style.[
+          position(`Relative),


Thanks for converting everything to this new paradigm!

bryphe · 2019-01-20T17:04:07Z

test/UI/StyleTest.re

+open Revery_Core.Colors;
+open Style;
+
+test("Style API tests", () => {


Nice! Thank you for adding some tests.

Do we have any tests exercising the 'inheritance' behavior? Like if I do something like Style.inherit([style1, style2, style3]), what the expected outcome is?

@bryphe I've implemented a merge function which I've added tests for it doesn't follow the signature you've laid out largely because I'm not really sure how to do that, took a fair bit of trial and error to get what I did but I'm sure there's a better/more performant way than my current solution (whatever it is I don't know it)

I could potential see how you could take a list of lists and maybe run merge on the first tow then use the result and run it on the next but tbh my current solution is not very efficient so using it that way is probably not ideal 🤕

I think its OK to start with what we have here, and then improve. We need proper tools to 'benchmark' anyway before we can truly optimize aggressively!

bryphe · 2019-01-20T17:05:56Z

src/UI/Style.re

+   -------------------------------------------------------------------------------*/
+
+type styleProps = [
+  | `Position(LayoutTypes.positionType)


Following from above - what's the benefit of using polymorphic variants here? Do "normal" variants not support this?

It sounds like one benefit is that you don't need to have the module prefixed with it, which is nice!

And the typos can be avoided using styleProps in the type annotation, it seems.

Polymorphic variants allow for reuse of the same names in different types, which is great if another library needs the same constructor names. But the way it does this is through subtyping, which means we can also have a hierarchy of style types. Really basic example:

type textProps = [ | `FontSize(int) | `FontFamily(string) ]; type borderProps = [ | BorderRadius(int) | Border(width, color) ]; type layoutProps = [ | `Width(int) | `Height(int) ]; type textNodeProps = [ layoutProps | textProps ]; type viewNodeProps = [ layoutProps | borderProps ];

What that lets us do is statically check that all applied props actually exist for the node you're providing them for. I.e. we can't do <view style=[fontSize(10)]/> but we can use that prop on text nodes. And then we can do <view style=[border(5, blue)]/> but that won't work for a text node.

Thanks for the details @OhadRau !

this is done now View nodes now report a type error if you try and pass them font props

bryphe · 2019-01-20T17:07:37Z

src/UI_Components/Clickable.re

-    let style2 =
-      Style.extend(style, ~opacity, ~cursor=MouseCursors.pointer, ());
+    /* let style2 = */
+    /*   Style.extend(style, ~opacity=animatedOpacity, ~cursor=MouseCursors.pointer, ()); */


Ah, I guess the 'inheritance' behavior hasn't been implemented yet - that piece would be nice to have with this PR (since it's one of the benefits of moving to this new model!)

bryphe · 2019-01-20T17:11:43Z

Looks great to me @Akin909 ! Thanks for the feedback / help @wokalski and @rauanmayemir.

It would be nice to move to a similiar API to other 'native' UIs, and the advantages you called out make sense to me! I'm looking forward to this simpler syntax (I always forget LayoutTypes.JustifyCenter, etc with the old API, and forgetting the last () unit parameter, etc - so I agree this is easier and more newbie friendly).

I'm on board - I think we just need to implement the 'merge' / 'inherit' behavior (and wire it up in the places that were using Style.extend previously) - otherwise I'm all set on this👍 Nice work!

wokalski

Hey, this PR looks great. Thanks for the hard work, it's nice to see that we're building one platform - with the same conventions and choices. It's really great. I have just one concern/question (and I think it might be as well about brisk.) Even though we create functions for the "top level" style definitions we still pass plain variants/records to the style functions which feels a little bit weird with such API and is harder to discover. Also, records are tricky in such case because if one does Style.borderHorizontal({width, color}) the fields width and color won't be qualified (from the type system's point of view.) I think we could do better by using labelled arguments for arguments like borderHorizontal(~width, ~color) and maybe constants for things like position, i.e. Position.absolute? I'm not 100% sure but I think it's something to think about.

Also, I'd like to know what you think about styles composition. Lists are nice but appending lists is O(n) which feels like a waste for such operation. We could create a tree where prepending would take constant time. We could still use list syntax like in this snippet

wokalski · 2019-01-20T20:36:00Z

examples/Border.re

-  );
+  Style.[
+    backgroundColor(Colors.yellow),
+    position(`Relative),


Variant as arg ☝️

Re. this one having had a look at brisk, bs-css and bs-nice it looked almost like this was the general pattern i.e. variants as args I think an advantage of this is that if there is well documented example then its quite intuitive (maybe not immediately clear what a variant is etc.) but you can start to see what you might need to pass in and how.

I think constants would require a user knew what needed to be openend (just Style probably), from my initial foray into reason I encountered that using bs-nice i.e. polymorphic variant as arg didn't exactly know what the syntax meant but was able to pretty easily guesstimate the pattern and the compiler errors also point towards the types of arguments so its easy to pick up.

If it might be an anti-pattern in reason/ocaml again happy to change although, in this vs the case of records I feel like this pattern is more approachable than finding the appropriate constant?

To be clear, I'm not 100% sure what's better, I'm just leaning towards the functional API (even though I was proposing to stick to plain variants at the beginning). There are two extremes:

Eveything's just a plain variant

We have functions/constants for everything.

In terms of code between this:

Style.[ backgroundColor(Colors.yellow), Position.relative, (or /*position(relative)*/) ...

and this

Style.[ `BackgroundColor(Colors.yellow), `Position(`Relative), ...

My case for the everything is a functional API is:

you can have optional args

Better documentation

Btw, records had the same "problem" (it's also an advantage) that you have to open the module which contains the records type.

wokalski · 2019-01-20T20:36:10Z

examples/Border.re

+    top(0),
+    width(30),
+    height(30),
+    borderHorizontal({color: Colors.black, width: 3}),


Record as arg ☝️

@wokalski thanks for the feedback 🙏 its much appreciated. Re. the records, I'm not sure I understand

the fields width and color won't be qualified (from the type system's point of view.)

The function is typed and the compiler errors if the wrong type is passed or any field is missing 😕, my thinking was the syntax for a record would be more easily recognisable to anyone new to reason, and in the layer beneath a record is expected and since they seemed to be 2 similar solutions 🤷‍♂️, if there is something lost by doing it with a record though I'm happy with changing it

The bottom line of my concern was that we create the styles using functions but pass other values to the functions instead of using other means like constants or labelled arguments instead. I think it might be just additional mental overhead. Compare:

borderHorizontal({color: Colors.black, width: 3})

borderHorizontal(~color=Colors.black, ~width=3)

The second version also gives good type errors:

borderHorizontal(3, Color.black)

would probably yield something about the function being expected of type ~color, ~width not (int, Color.t) instead of just telling that you gave two arguments instead of one "border" record. I might be missing some advantages of the current API though.

Another advantage of not using records is that you don't have to open the Style module and you can make optional parameters. To get optional parameters you'd have to do something like borderHorizontal({...defaultBorder, color: Colors.black, width: 3}) which would be kinda annoying.

happy to change to the labeled args syntax although theres quite a few variations of the border so don't think any of them will be optional, been trying to avoid the optional syntax as I think forgetting to pass unit is a pretty common issue people will hit with functions like that at least at the beginning almost offsetting the value of having things be optional.

tcoopman · 2019-01-20T21:56:43Z

Also, I'd like to know what you think about styles composition. Lists are nice but appending lists is O(n) which feels like a waste for such operation. We could create a tree where prepending would take constant time. We could still use list syntax like in this snippet

Just a question when I read your remark. If something like this is actually changed, shouldn't there be some benchmarks to validate that the change is actually improving things?

wokalski · 2019-01-20T22:03:53Z

@tcoopman of course - benchmarks would be great but we're on the edge here with extremely scarce resources. I have a similar situation in the reconciler where I'd love to have some micro benchmarks already to have some sense of the impact of my changes but I don't. Also - I don't think this part of styling is significant in terms of performance or maybe there are situations where it is but it's probably not easy to measure. My comment is kind of more about the culture of what we do. I'd like us to always keep in mind the performance characteristics because small things do add up!

tcoopman · 2019-01-20T22:24:39Z

@wokalski I totally agree and I would love if performance is a first class concern all the time. In this case though I was actually wondering if the the custom tree implementation would be faster than the list in practice.

I don't have any experience with benchmarking and improving OCaml code and have no idea in what code paths the styles will have to be merged a lot, so I might be completely off here, but the reason I was wondering was because I already saw a case myself where I thought a list implementation would be slower than a map, but in practice the difference was only noticeable from bigger lists (I had an implementation of the Game Of Life that worked with lists and with maps). I don't remember how big the lists had to be before the performance started to go into favor of maps. But my point here is that just changing implementations without benchmarks might not improve things.

wokalski · 2019-01-20T22:39:34Z

@tcoopman good point!

Also use in clickable function to allow for inheritance

so each only uses props appropriate to it

akinsho · 2019-01-22T00:16:13Z

@bryphe any chance we can hold off or at least slowdown style related PRs till this is in, every time theres a new style added, I have to rewrite all the associated styles 😢 its mega time consuming 🤣

I have to finish refactoring the latest updates but don't have too much left to do

bryphe · 2019-01-22T00:30:10Z

Ah sorry @Akin909 ! Ya that would be brutal to merge everything. Sure thing, I'll hold off on any new PRs for now until we get this in 👍

bryphe · 2019-01-22T17:50:49Z

Thanks for your work on this, @Akin909 ! Is this good to go, or is there any work remaining?

wokalski · 2019-01-22T20:56:10Z

Looks good to me 👍

akinsho · 2019-01-22T23:05:41Z

@bryphe I think given the size of this PR already this is a good place to finish 👍 , but I have a follow up additional change I was going to add to simplify some of the functionality of working with list of styles, specifically around being able to unwrap a style e.g. in the input if a user passes a height of x then simplify the process of getting the value which at that point is wrapped in a variant but that can definitely come separately

bryphe · 2019-01-22T23:11:33Z

@Akin909 - sounds good! Let's bring this in so you don't have to deal with anymore merge craziness 😆 and then we can track the remaining issues as follow-up items and PRs.

Thanks for all the hard work on this! And thanks all for the reviews and feedback!

bryphe

🎉 🎉

jordwalke · 2019-01-22T23:14:32Z

What about the additional allocations here? (I'm assuming there are some)

akinsho · 2019-01-22T23:19:08Z

@jordwalke I'm not sure I understand?

bryphe · 2019-01-22T23:30:39Z

@Akin909 - I believe @jordwalke means memory allocations (allocations that can impact the GC). This can be troublesome for performance

We definitely need tooling to validate this - I just logged an issue to track - #240. But I think what will be important is to come up with some 'benchmark' scenarios that we can test and measure any timing / allocation impact.

akinsho added the WIP label Jan 19, 2019

wokalski reviewed Jan 19, 2019

View reviewed changes

rauanmayemir reviewed Jan 19, 2019

View reviewed changes

akinsho force-pushed the feature/style-props-as-list branch from 2fa9614 to 6ebe082 Compare January 20, 2019 15:13

bryphe reviewed Jan 20, 2019

View reviewed changes

examples/Border.re Show resolved Hide resolved

bryphe reviewed Jan 20, 2019

View reviewed changes

wokalski reviewed Jan 20, 2019

View reviewed changes

akinsho added 14 commits January 21, 2019 23:13

[WIP] add very early implementation of styles as a list

61dca1a

Add more test of more properties

9289dbe

Change border function to take a record type

b83231c

Begin refactor to list API

39789dc

Refactor examples to new API [WIP]

e427b47

Convert even more examples...

6255624

Finish refactoring examples

56915d8

Fix styles not being passed in calculator example

ea6cbdb

Fix broken style tests and make coord types general

c9ff3cc

Update args for border creation function

0f3530f

Change border api to take labelled args

a12a233

Convert margin api to use labelled args

3d8d7f1

Add merge function and update tests

d3c2d85

Also use in clickable function to allow for inheritance

Fix type issues with baseProps variants

52b12be

akinsho added 4 commits January 21, 2019 23:15

Rename back to props as base props isn't accurate

73d8a82

Type view and text nodes

10c32bd

so each only uses props appropriate to it

Remove unused extend function and fix typos in comments

bea583a

rebase master, refactor all the new things 😢

bf87e57

akinsho force-pushed the feature/style-props-as-list branch from fdbb814 to bf87e57 Compare January 22, 2019 00:13

Refactor new components to use new style api

7a64104

akinsho changed the title ~~[WIP] Feature/Update Style API~~ Feature/Update Style API Jan 22, 2019

akinsho removed the WIP label Jan 22, 2019

bryphe approved these changes Jan 22, 2019

View reviewed changes

akinsho merged commit bedd175 into revery-ui:master Jan 22, 2019

akinsho deleted the feature/style-props-as-list branch January 22, 2019 23:14

akinsho mentioned this pull request Jan 22, 2019

Convert Box shadow to use labelled args #241

Closed

akinsho mentioned this pull request Jan 24, 2019

update image primitive to use new style api #257

Merged

Feature/Update Style API #215

Feature/Update Style API #215

Conversation

akinsho commented Jan 19, 2019 • edited Loading

Advantages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

bryphe Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Jan 20, 2019 • edited Loading

wokalski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wokalski Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcoopman commented Jan 20, 2019

wokalski commented Jan 20, 2019

tcoopman commented Jan 20, 2019

wokalski commented Jan 20, 2019

akinsho commented Jan 22, 2019 • edited Loading

bryphe commented Jan 22, 2019

bryphe commented Jan 22, 2019

wokalski commented Jan 22, 2019

akinsho commented Jan 22, 2019

bryphe commented Jan 22, 2019

bryphe left a comment

Choose a reason for hiding this comment

jordwalke commented Jan 22, 2019

akinsho commented Jan 22, 2019

bryphe commented Jan 22, 2019

akinsho commented Jan 19, 2019 •

edited

Loading

bryphe Jan 20, 2019 •

edited

Loading

bryphe Jan 20, 2019 •

edited

Loading

bryphe Jan 20, 2019 •

edited

Loading

bryphe commented Jan 20, 2019 •

edited

Loading

wokalski Jan 21, 2019 •

edited

Loading

akinsho commented Jan 22, 2019 •

edited

Loading