add input component and example #205

akinsho · 2019-01-12T18:20:34Z

This is a v. basic implementation of an input component there's a fair bit that could be added (down the line I think) like some padding, Events (mouse, focus etc), but this is a starting point for that.

Updated

Position cursor on the left when placeholder is present
Add padding so text isn't flush with border
Added text cursor
Added mouse focusing
add props to allow for more styling

Issues/Bugs discovered

I've found that trying to use an effect based on state, when the effect has be specified as on mount means the state value is in a closure I think, though switching to Always means that a handler is never missed, example

let ... = React.Hooks.effect(
  Always, /* if always this handler overrides any other handler registered for the same event*/
  (event) => 
    Some(
     Event.subscribe(
       window.keyDown,
       ()  => {
        if (state.isFocused) {
          onChange(state.value) /* on mount this value is old */
         }
       }
    )
  )

akinsho · 2019-01-13T00:58:43Z

The reconciler is I think behaving oddly as when I try and set state in the example component it doesn't get updated correctly

bryphe · 2019-01-15T02:32:56Z

src/UI_Components/Input.re

+                  ~children,
+                  ~window,
+                  ~color=Colors.black,
+                  ~fontSize=18,


For some of these properties (fontSize, color, etc) - would it make it sense to take in a style instead? I think the tricky thing is letting a user set Style but having a set of default values that aren't set (and if they aren't set, they get overridden) - we don't have a good solution for that yet (but we should)

For example, I could see customizing the fontFamily, border, or backgroundColor as common scenarios here too.

Have you thought about using polymorphic lists instead of records for style? You could then have a common set of props and extra props per component. (and do not set values for props that are not set)

@rauanmayemir @bryphe I've been thinking about that a lot recently (was going to raise an issue) as I've used https://github.com/SentiaAnalytics/bs-css which looks like -

let card = style([ display(flexBox), flexDirection(column), alignItems(stretch), backgroundColor(white), boxShadow(~y=px(3), ~blur=px(5), rgba(0, 0, 0, 0.3)), /* You can place all your theme styles in Theme.re and access as normal Reason module */ padding(Theme.basePadding) ]);

and bs-nice which has an even nicer API imo

let className = css([| Display(Flex), AlignItems(Center), Color(Red), Select(":hover", [Color(Blue)]) |]);

css is sort of odd when it comes to styling because all style properties are accepted by all components.

Polymorphic variants allow us to have one style (a list) used by several components at the same time, as long as the list only contains styles that are shared by them. Taking Input and View as an example this would work: [`BackgroundColor(c), `Width(300.)] but this would not [`BackgroundColor(c), `ContentsAspectRatio(x)] because aspect ratio does not make sense for View. The actual api can be a little different, i.e. we could use functions like backgroundColor(c) but I'm talking about types specifically now.

quite new to reason but could the style function take something like

style(ViewStyles, [/* list specific to view styles */])

and have the first argument somehow 🤔 😕 inform the type of the second, or just have different style functions for what I imagine will be a small subset of components 🤷‍♂️ . Although so far we don't really have that kind of variation in what nodes expect, also our current api allows for that i.e. Style.make(// all possible style arguments

@Akin909 it could! Let me elaborate what we meant in more detail. When you define a shared style like let highlightedText = [`Color(red)] you might want to use it for both an Input and normal Text. However, `Color does not make sense for a View for instance, which already has the background color style. Because of that I'd like to get a type error when I pass such style to a View. Now, non-polymorphic variants are nominally typed, i.e. even if you have two types like this:

type a = | A; type a' = | A; let f = (x: a) => x let g = (x: a') => f(x);

g won't compile because the two variants have nominally different types despite being identical constructors. Polymorphic variants are type checked according to their constructors so we could pass the same list with the `Color style for multiple components even though each of them has its own style type.

bryphe · 2019-01-15T02:33:27Z

src/UI_Components/Input.re

+                  ~width=200,
+                  ~height=50,
+                  ~value=None,
+                  ~placeholder="",


That's really neat that there is placeholder text - just tried out the demo 💯

bryphe

Looks great to me overall! This is awesome @Akin909, thank you!

Just had a question about the plan for styling this custom component. I'm wondering how we can handle custom styling on some of these built-in components, and I think it will be particularly important for this one.

I think having a place where we can start to invest in these components is important - it's so cool to see a functional <Input /> component (with blinking cursor and everything!) come together.

akinsho · 2019-01-16T22:39:53Z

@bryphe I've played around with this a little trying to work with Styles as we have them and updating and changing them in reason, and the core issue (a language feature not a real issue) is that non of the Style fields are actually falsy and trying to do a merge which overwrites all the undefined fields would either require an enormous pattern matching or ... 🤷‍♂️ (I can't see how else to do it as I essentially need to check each field of both records and if a value is the falsy/undefined one overwrite it)

Not sure where you land on this but I think a list of functions or types as discussed above is a much easier to work with api, but would involve a larger scale change (if you agree), in which case for the time being I can pass in a subset of the style props optionally for this PR and look at the bigger change separately

akinsho · 2019-01-19T02:02:55Z

@bryphe I've made some tweaks to this in response to your feedback and commented a bit above about the why I've gone this way a bit more. Lemme know what you think

bryphe · 2019-01-20T16:38:24Z

examples/Examples.re

@@ -97,11 +97,18 @@ let reducer = (s: state, a: action) => {
 let init = app => {
  let maximized = Environment.webGL;

+  let size = Monitor.getPrimaryMonitor() |> Monitor.getSize;


Is it necessary to change the example size for this? I'd like to keep the size "as-is" and then showcase some capabilities like fullscreen/maximize/extra windows via #223 and #224

@bryphe no worries can change that back. On my machine it opens as a really small window in the corner and when I go to maximize it crashes due to the resize bug, thought it was a "bug" that it didn't open with a larger size

Reverted 👍 @bryphe

Hmm, strange - sounds like a bug! Would you mind logging an issue with a screenshot? Would be nice to also include the display + machine details. There might be other high-dpi issues that haven't been addressed - hopefully we can address them in a holistic way.

Will do 👍

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

src/UI_Components/Input.re

+    /*
+       TODO: Setting the hook to run only on mount means that the onChange
+       handler is only called with a stale version of state
+       BUT setting the value to ALWAYS causes one handler to to seize


Interesting! This sounds like a bug. What does it mean by causes one handler to seize control of event listening?

The other handler doesn't respond at all it's almost as if only one handler is registered, I only noticed when I added the second input example basically with Always set the key events registered for any other component that implements a handler for the same event aren't called / don't respond.

bryphe · 2019-01-20T16:42:51Z

src/UI_Components/Input.re

+    ) =>
+  component(slots => {
+    let initialState = {value, placeholder, isFocused: false};
+    let (state, dispatch, slots) =


Nice use of the new hooks API!

bryphe · 2019-01-20T16:43:30Z

src/UI_Components/Input.re

+        <Text style=innerTextStyles text=content />
+        /*
+           TODO:
+           1. Show and hide cursor based on focus


Nice progress!

👍 The cursor bit is actually tied to having mouseover and mouseout, what I meant by the remaining comment there is that the mouse cursor currently responds to clicks but not to mousing over (although that's not actually browser behaviour so think the comment is invalid)

I just checked and the cursor doesn't appear or disappear in a browser based on mousing over or out only as a result of clicking in or out

bryphe · 2019-01-20T16:55:36Z

Sorry for the delay on reviewing this @Akin909 ! The <Input /> control looks great to me - excellent starting point and we can continue to refine based on the TODOs you mentioned.

Thank you for thinking about the Style issue and adding a new PR / issue for it - definitely makes sense to split out - and I like the direction you're investigating 👍

I only had one piece of feedback - can we leave the example size as-is for now? Or is there a reason it needs to be maximized for the sample?

Thanks for the work on this! It's great progress (will be very useful for Oni2 also - we need this for the command palette / quick open / etc)

Remove no longer relevant comment from input

This fixes the input component example which #205 broke, it involves having to pull values out of the style list which I wanted to create a sort of *getter* for but having done a bit of reading and wrangling with the compiler doesn't seem straight forward, typing gets a little tricky with variants will keep it in mind and revisit once I've leveled up my reason/ocaml know-how (tracked by #239)

akinsho added the WIP label Jan 12, 2019

akinsho removed the WIP label Jan 13, 2019

bryphe reviewed Jan 15, 2019

View reviewed changes

bryphe requested changes Jan 15, 2019

View reviewed changes

akinsho force-pushed the feature/add-input-component branch 2 times, most recently from 59d262b to 8b56a46 Compare January 18, 2019 23:47

akinsho force-pushed the feature/add-input-component branch from 40dc007 to 8d29986 Compare January 19, 2019 11:12

akinsho mentioned this pull request Jan 19, 2019

Feature/Update Style API #215

Merged

bryphe reviewed Jan 20, 2019

View reviewed changes

akinsho added 13 commits January 20, 2019 18:12

add input component and example

5de4179

Fix placeholder functionality

710fd71

Improve input example

437b2c6

Add padding, make component controllable

879a2cd

Add comment explaining top padding

10141d1

Actually commit addition of left margin to cursor

5ca07fa

Fix width so there is no jank on typing

6f81172

WIP: extensible styling

0b56b58

pass down props to configure input rendering

a2bf01a

Do nothing if the input does not have focus

ae9ab16

[WIP] convert this to the new slots API

13bda25

Add createElement to the input example

79c3dc2

Neaten hooks logic in input

b80d918

akinsho added 7 commits January 20, 2019 18:12

Add example to examples and tidy up input example

385f505

Fix input example [WIP]

f44135d

Return to using onMount as always leads to bugs

3d96549

Remove unnecessary width prop from inner text

faafc9b

Make example app bigger on opening natively

cb6add4

Fix comment typo and remove old comment

03fd415

Revert changes to example app sizing

d6e382e

Remove no longer relevant comment from input

akinsho force-pushed the feature/add-input-component branch from 6640759 to d6e382e Compare January 20, 2019 18:15

bryphe approved these changes Jan 20, 2019

View reviewed changes

akinsho merged commit 6b9c196 into revery-ui:master Jan 20, 2019

This was referenced Jan 20, 2019

Small window size on HiDPI screen #226

Closed

UI - Input Component #203

Closed

Bugfix/fix input example #256

Merged

bryphe mentioned this pull request Jan 30, 2019

UI - Input: Hook up <Input /> element to Focus #290

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add input component and example #205

add input component and example #205

akinsho commented Jan 12, 2019 •

edited

Loading

akinsho commented Jan 13, 2019

bryphe Jan 15, 2019

rauanmayemir Jan 15, 2019

akinsho Jan 15, 2019 •

edited

Loading

wokalski Jan 15, 2019 •

edited

Loading

akinsho Jan 16, 2019 •

edited

Loading

wokalski Jan 17, 2019

bryphe Jan 15, 2019

bryphe left a comment •

edited

Loading

akinsho commented Jan 16, 2019

akinsho commented Jan 19, 2019

bryphe Jan 20, 2019

akinsho Jan 20, 2019

akinsho Jan 20, 2019

bryphe Jan 20, 2019 •

edited

Loading

akinsho Jan 20, 2019

bryphe Jan 20, 2019

akinsho Jan 20, 2019

bryphe Jan 20, 2019

bryphe Jan 20, 2019

akinsho Jan 20, 2019

akinsho Jan 20, 2019

bryphe commented Jan 20, 2019

add input component and example #205

add input component and example #205

Conversation

akinsho commented Jan 12, 2019 • edited Loading

Updated

Issues/Bugs discovered

akinsho commented Jan 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akinsho Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

wokalski Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

akinsho Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe left a comment • edited Loading

Choose a reason for hiding this comment

akinsho commented Jan 16, 2019

akinsho commented Jan 19, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Jan 20, 2019

akinsho commented Jan 12, 2019 •

edited

Loading

akinsho Jan 15, 2019 •

edited

Loading

wokalski Jan 15, 2019 •

edited

Loading

akinsho Jan 16, 2019 •

edited

Loading

bryphe left a comment •

edited

Loading

bryphe Jan 20, 2019 •

edited

Loading