[0.62] add accessibilityState prop and constructor for related object by sgny · Pull Request #657 · rescript-react-native/rescript-react-native

Conversation

@sgny

This PR adds the replacement prop for accessibilityStates removed in 0.62.

Before merging, I'd appreciate some feedback on the API. I have chosen to add a module called Accessibility and a type called state with a constructor of the same name. I plan to add another type called value and again a constructor of that name to handle the accessibilityValue prop which also seems to be missing from our bindings.

The JS API necessitates some rather unpleasant choices for the checked field, which can take either a bool or the string "mixed". Since unwrap is still unavailable, this PR defines a type checked in Accessibility, a function to type cast bool to checked and uses bs.inline to handle "mixed".

MoOx

MoOx previously approved these changes Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in practice this will give

<View accessibilityState=(Accessibility.(state(~disabled=true, ~checked=setChecked(true)))) />

<View accessibilityState=(Accessibility.(state(~disabled=true, ~checked=mixed))) />

<View accessibilityState=(Accessibility.(state(~disabled=true, ~checked=setChecked(false)))) />

That's zero cost but not very fancy.

I don't see another zero cost option.

We could offers something like

<View accessibilityState=(Accessibility.(state(~disabled=true, ~checked=checked))) />

<View accessibilityState=(Accessibility.(state(~disabled=true, ~checked=mixed))) />

<View accessibilityState=(Accessibility.(state(~disabled=true, ~checked=unchecked))) />

But I don't think we can make this zero cost, even if checked & unchecked are functions...

So I guess documentation should be enough for people to find the way to handle this. Maybe we could start with an Accessibility guide with some examples?

@sgny

@MoOx

I agree, zero cost does not seem possible with a significantly nicer API, at least for now while @bs.unwrap is unavailable for use with @bs.obj. @bs.unboxed does not seem to improve on our "%identity" tricks in this regard.

With that said, we could also define

type checked = bool;

let mixed: checked = "mixed"->Obj.magic;

as we have already used elsewhere in this repo. This is again quite low cost.

Then, instead of inlining as string, we could use inlining as bool, as below:

 [@bs.inline]
 let checked = true;

 [@bs.inline]
 let unchecked = false;

An appropriate interface would be needed, but perhaps this version would be nicer?

@MoOx

Would let mixed: bool = "mixed"->Obj.magic; be possible (so no checked/unchecked + interface) required?

@sgny

@MoOx It's certainly possible, but then mixed can be passed to all fields.

@MoOx

Would you want to pass mixed as a boolean everywhere? But yeah this could lead to a runtime error. I am ok for your last proposal then.

MoOx

MoOx approved these changes Mar 27, 2020

@MoOx MoOx deleted the accessibilityState branch

March 27, 2020 21:28

2 participants

@sgny @MoOx