Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add mapWithIndex & forEachWithIndex to Children #515

Merged
merged 2 commits into from Apr 17, 2020

Conversation

cem2ran
Copy link
Contributor

@cem2ran cem2ran commented Apr 7, 2020

Needed the array index when iterating through the children prop so I added bindings for map & forEach that follow the naming convention in Belt (suffixed WithIndex).

Example usage:

let children =
      switch (gap) {
      | Some(spacing) =>
        children->React.Children.mapWithIndex((element, idx) =>
          idx > 0 ? <> <Spacer amount=spacing /> element </> : element
        )
      | None => children
      };

@rickyvetter
Copy link
Contributor

I'm nervous about this because using index as an id for dynamic children is so tempting and become that much easier with this. That said, I think mapping to the existing React apis make sense. Shouldn't hide this functionality.

@cem2ran
Copy link
Contributor Author

cem2ran commented Apr 15, 2020

Valid concern. Seems like something that should be a React runtime warning rather? Incorrect usage of React rather than a bs-typing/library issue.

The alternative without mapWithIndex would be something like this I assume, which is a bit wordy/noisy:

children->React.Children.toArray->Belt.Array.mapWithIndex((idx, element) =>
  idx > 0 ? <> <Spacey amount=spacing /> element </> : element
)->React.array

@cem2ran
Copy link
Contributor Author

cem2ran commented Apr 15, 2020

Another note: While this PR follows the withIndex convention from Belt, the ordering of Array.map & Children.map arguments are reversed.

Array.mapWithIndex: (int, element)
Children.mapWithIndex: (element, int)

I'm thinking this is OK, but I wanted to highlight this mismatch.

@cem2ran
Copy link
Contributor Author

cem2ran commented Apr 16, 2020

Made the changes. I'm a bit unsure about the formatting in this file. I'm currently seeing refmt change quite a lot of lines, so I decided to only commit the unformatted lines related to my changes:

diff --git a/src/React.re b/src/React.re
index 23666465..9ac7dbb3 100644
--- a/src/React.re
+++ b/src/React.re
@@ -11,10 +11,12 @@ type componentLike('props, 'return) = 'props => 'return;
 type component('props) = componentLike('props, element);
 
 [@bs.module "react"]
-external createElement: (component('props), 'props) => element = "createElement";
+external createElement: (component('props), 'props) => element =
+  "createElement";
 
 [@bs.module "react"]
-external cloneElement: (component('props), 'props) => element = "cloneElement";
+external cloneElement: (component('props), 'props) => element =
+  "cloneElement";
 
 [@bs.splice] [@bs.module "react"]
 external createElementVariadic:
@@ -28,18 +30,21 @@ module Ref = {
   [@bs.set] external setCurrent: (t('value), 'value) => unit = "current";
 };
 
-[@bs.module "react"] external createRef: unit => Ref.t(Js.nullable('a)) = "createRef";
+[@bs.module "react"]
+external createRef: unit => Ref.t(Js.nullable('a)) = "createRef";
 
 module Children = {
   [@bs.module "react"] [@bs.scope "Children"] [@bs.val]
   external map: (element, element => element) => element = "map";
   [@bs.module "react"] [@bs.scope "Children"] [@bs.val]
-  external mapWithIndex: (element, [@bs.uncurry] ((element, int) => element)) => element =
+  external mapWithIndex:
+    (element, [@bs.uncurry] ((element, int) => element)) => element =
     "map";
   [@bs.module "react"] [@bs.scope "Children"] [@bs.val]
   external forEach: (element, element => unit) => unit = "forEach";
   [@bs.module "react"] [@bs.scope "Children"] [@bs.val]
-  external forEachWithIndex: (element, [@bs.uncurry] ((element, int) => unit)) => unit =
+  external forEachWithIndex:
+    (element, [@bs.uncurry] ((element, int) => unit)) => unit =
     "forEach";
   [@bs.module "react"] [@bs.scope "Children"] [@bs.val]
   external count: element => int = "count";
@@ -63,7 +68,8 @@ module Context = {
     "Provider";
 };
 
-[@bs.module "react"] external createContext: 'a => Context.t('a) = "createContext";
+[@bs.module "react"]
+external createContext: 'a => Context.t('a) = "createContext";
 
 [@bs.module "react"]
 external forwardRef:
@@ -310,7 +316,8 @@ external useCallback7:
   callback('input, 'output) =
   "useCallback";
 
-[@bs.module "react"] external useContext: Context.t('any) => 'any = "useContext";
+[@bs.module "react"]
+external useContext: Context.t('any) => 'any = "useContext";
 
 [@bs.module "react"] external useRef: 'value => Ref.t('value) = "useRef";

@rickyvetter
Copy link
Contributor

Yup thanks! We can format in a separate pass.

@rickyvetter rickyvetter merged commit fa3df2d into reasonml:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants