belle + radium bug

[luisrudge]

Not sure where's the problem, so I'm cross posting this in the two repos. [https://github.com/nikgraf/belle/issues/170](Belle repo)

When using Radium with Belle, I get this error: Uncaught TypeError: iterable.forEach is not a function

Here's the repro: https://github.com/luisrudge/belle-radium-bug

[jpuri]

I debug the issue in radium + belle, and it came out that out 2 components - Select / ComboBox are not working well with radium. These components are structured like:

<Select>
  <Placeholder>Select Country</Placeholder>
  <Option value="USA">USA</Option>
  <Option value="UK">UK</Option>
</Select>

Select code expects to get an array of the nested components in props.children:

But with radium this array is converted to object:

This is breaking the 2 components :(

[AnSavvides]

The error itself is in belle's source, in particular in helpers.js. The filter util is used inside both ComboBox and Select.

It looks like the iterable parameter passed to filter is not an Array, hence bad things happen.

@jpuri and @nikgraf are probably better suited to figure out what's happening exactly from belle's side of things!

[jpuri]

Hey @AnSavvides , yep the error is coming from helpers.js. But helpers.js is expecting to receive an array. In fact props.children do give us array when we use Select and ComboBox.

When with radium props.children is getting converted to object and filter is breaking as its not made to filter Object values. That conversion from Array to Object in first place should ideally not happen.

Both Select and ComboBox are working perfect without Radium.

[ianobermiller]

This isn't a Radium bug; belle should never manipulate this.props.children directly (e.g. by expecting it to be an array). Only the React.Children helpers should be used: https://facebook.github.io/react/docs/top-level-api.html#react.children

[jpuri]

This makes sense, we will refactor our code for that. Thanks a lot @ianobermiller.

[nikgraf]

@AnSavvides @ianobermiller We fixed the issue and cut a new release. The newest version works well with Radium 😄

[luisrudge]

Awesome work all of you!

[ianobermiller]

@nikgraf Great to hear, thanks for the work everyone! Btw, Belle is really nice! Have you thought about building it using Radium? Would take away some of the repetitiveness.

Do you have any tips on touch support for components? Radium adds mouseEnter and mouseLeave for hover effects, for instance, which I believe will cause the dreaded double-touch on mobile (I could be totally wrong about this, though).

[nikgraf]

@ianobermiller Yes, we considered Radium before starting out. To be honest the whole styles in JS topic is constantly on my mind. I really like Radium and want to give it a try for a project.

One reason we didn't go for Radium is that there are many people out there using different ways of styling React Components. You can use Radium, React-JSS, React-inline and many more. If we would pick one of them for Belle, this might pre-subscribe developers to this particular styling tool. I'm not sure how well they work together and how much overhead they create.

Another reason was that for more complex components we needed to implement styles like activeStyle as inline styles, because the pseudoClass :active won't work on a div.

[nikgraf]

@ianobermiller here are some thoughts on touchEvents:

Observed behaviour in browsers for a default :

Browser on Desktop

When entering the button area with the mouse the components goes into :hover state.
When pressing the button (mouseDown) the button goes into :active state and is still in :hover state. Even when you leave the button with the mouse pointer, but still keep it pressed it's still in active state. I find this really confusing, because if you release the mouse outside (mouseUp) the button onClick will not be triggered. If you release the mouse inside the button it will be triggered.

Browser on Mobile

When you touch a button nothing happens, because the Mouse Events are only fired after the touch sequence ends (see https://github.com/jquery/PEP#why-pointer-events). Once touchStart & touchEnd fired on the same button a series of mouse events will fire (mouseDown, mouseUp, onClick). The button will remain in :hover state. I'm probably missing something, but to me this behaviour doesn't make sense.

Behaviour with Belle

On desktop we simply implemented the same default behaviour. I thought active style doesn't work for native buttons, but it actually does we just could save 2 lines of code :)
I considered removing the :active state when a user leaves the button while the mouse is pressed down. This would have introduced a certain amount of complexity. From a my perspective it would make more sense, but on the other hand we weren't sure if we should break with the default behavior here.

For :hover state we simply used the :hover pseudoClass, but I noticed this causes trouble on Mobile. We probably should follow the Radium's way by doing mouseEnter & mouseLeave. Did you ever run into trouble just relying on those e.g. edge cases with screen readers or so on?

TouchEvents in general

I learned that it's quite an effort to make components behave intuitively on touch devices. For example for the Rating to allow touching the component and then slide to the right to increase the rating you need to use touchMove and then get the element from the client position. Events like mouseEnter don't exist for touch (see https://github.com/nikgraf/belle/blob/master/src/components/Rating.jsx#L328)

[ianobermiller]

Thanks for the great comment on touch!

Did you ever run into trouble just relying on those e.g. edge cases with screen readers or so on?

Never tested with screen readers, I never thought about what it would mean for them.

I learned that it's quite an effort to make components behave intuitively on touch devices.

I noticed Belle has quite a bit of code for dealing with touch events, well done. It is hard space to get correct, and I think Radium could do more in this area.

Another reason was that for more complex components we needed to implement styles like activeStyle as inline styles, because the pseudoClass :active won't work on a div.

FWIW, Radium writes pure inline styles, and doesn't use the :active pseudo-class of CSS. We probalby do the same thing you do.

[nikgraf]

Ahhh, didn't know. I need to dig deeper into Radium 😄