Allow further customization by allowing component replacement

[Nantris]

Ask your Question

I'm trying to implement custom styles for when users use dark/light mode and the system is set to the opposite color scheme - but I've hit a wall with trying to style the inputs. Is there any way to modify the text color of the labels? Right now we have black on dark gray, but would like to change that to white without having to fork the entire package to do it.

[Nantris]

I think that a simpler approach may be to allow users to pass in their own Text or Input components or what have you. That way the API surface could be minimized and users could have complete control. It should be easy to allow that. Would you be open to a PR for that @mmazzarolo?

[mmazzarolo]

@slapbox Yeah, I also prefer letting user pass down theit own component... which, technically you can already do. The container allows you to pass down any component (but it does some checks on the component name/type to determine the style)

[Nantris]

Thanks for your reply @mmazzarolo!

I hadn't thought to customize as you've mentioned, but that makes sense! I was thinking about a slightly different implementation that would allow users to benefit from your hard work more, with less custom code required for end users.

For example, users could pass their own custom input to Dialog.Input in a file in their project and then re-export their custom Dialog.Input

const Input = props => {
  return (
    <DialogDefault.Input
      labelComponent={CustomLabelComponent}
      inputComponent={CustomInputComponent}
      {...props}
    />
  );
};

export const Dialog = {
  Input,
  Container,
  // ... etc
}

That way all of the default styling you've worked so hard to make match the native OS dialogs would still be applied to the custom components.

Sort of like what React-Select does, but of course with many fewer options than they offer.

Thoughts?

---

Alternatively, if there were a way to force isDark values with a prop, that would be much more simple and would work for our use case.

[Nantris]

@mmazzarolo how does that switch statement you linked to actually work?

I've tried implementing custom components in line with your suggestion but I can't get them to end up anywhere other than otherChildrens

I try setting .name and .displayName, naming the function itself to match, but nothing seems to work.

If the switch compares the values against the imported components in Container.tsx and manually setting those properties isn't enough, then how can one actually provide custom components?

The fact the children end up in otherChildrens doesn't actually seem cause any issues until you try to implement buttons, and that turned out to be the reason behind #140 - but I'm not sure how to fix that still without fully forking the package or abandoning our desire to have the dialogs match the app theming.

Any advice would be hugely appreciated! Thank you again for this great package.

[mmazzarolo]

That way all of the default styling you've worked so hard to make match the native OS dialogs would still be applied to the custom components.
Thoughts?

Yeah. That totally makes sense to me, and it's something I thought about in the past. I just wasn't able to find a nice API to suit this use case.
I'm doing something similar in: https://github.com/mmazzarolo/react-native-modal-datetime-picker
I'm open to suggestions on the API level and on how to actually implement it.
(Please notice that it's very difficult for me to work on RN lately :/, I'm super busy this year -- but I'm open to help and guide if needed!).

I've tried implementing custom components in line with your suggestion but I can't get them to end up anywhere other than otherChildrens

Sorry, I'm missing one thing: is the component being part of otherChildren a problem for you?

[Nantris]

Sorry, I'm missing one thing: is the component being part of otherChildren a problem for you?

Unfortunately yes, as it breaks the side-by-side buttons on Android. If not for that, it would be entirely workable.

I'm doing something similar in: https://github.com/mmazzarolo/react-native-modal-datetime-picker

That looks interesting! We'll probably end up using it!

Since you already have implemented a similar API, but you say you weren't able to find a nice API for this use case for react-native-dialog, I wonder what your concerns were with implementing something like that in this project?

(Please notice that it's very difficult for me to work on RN lately :/, I'm super busy this year -- but I'm open to help and guide if needed!).

Very appreciative of all of your support and guidance! I'll be happy to work on this if we can find a good path forward, but I'm not well versed in Typescript so my work would likely need some cleanup before it can be merged.

I think we should prioritize in these terms:

1. If we can toggle dark/light mode with a prop like isDark, that seems ideal in terms of minimizing the API surface and minimizing maintenance.
2. If we can't get a simple toggle working, then discuss further how to allow custom components

Do you have any ideas why my isDark experiment might not have worked? I'm guessing this is something to do with PlatformColor but I really don't know - maybe I just made a mistake? Like in this line, is this always going to be the color for the current system mode?

react-native-dialog/src/Container.tsx

Line 176 in 0df53e3

backgroundColor: PlatformColor("?attr/colorBackgroundFloating"),

If we go for the custom component route, I'm thinking that a user would pass the custom components like in the code above

[Nantris]

Actually, I'm thinking I screwed up yesterday in my testing of whether setting isDark works. I think it will work, but passing it as a prop might be a bit messy messy since useTheme returns its own isDark value too.

[Nantris]

Nope, unfortunately something else seems to be at issue. Maybe you can tell what?

I made a branch on my fork with the code changes I thought would be necessary for supporting an isDark prop, but it doesn't seem to work: https://github.com/Slapbox/react-native-dialog/tree/darkModeProp

[Nantris]

Is there any way around our buttons ending up in otherChildren until I can figure out how to force isDark and submit a PR for that? I tried setting the component's displayName to "DialogButton" to match this line but it doesn't have any effect and I'm not sure why. I'd expect the components to be recognized as DialogButtons in the linked code below, but they're not.

Maybe a simple tweak to this code would be the easiest to allow customization for now?

react-native-dialog/src/Container.tsx

Lines 63 to 90 in 0df53e3

	React.Children.forEach(children, (child) => {
	if (typeof child === "object" && child !== null && "type" in child) {
	switch (child.type) {
	case DialogTitle:
	titleChildrens.push(child as TitleElement);
	return;
	case DialogDescription:
	descriptionChildrens.push(child as DescriptionElement);
	return;
	case DialogButton:
	if (Platform.OS === "ios" && buttonChildrens.length > 0) {
	buttonChildrens.push(
	<View
	style={[
	verticalButtons
	? styles.buttonSeparatorVertical
	: styles.buttonSeparatorHorizontal,
	buttonSeparatorStyle,
	]}
	/>
	);
	}
	buttonChildrens.push(child as ButtonElement);
	return;
	}
	}
	otherChildrens.push(child);
	});

[mmazzarolo]

Hola @slapbox !
Are you saying that adding the displayName like we're doing here is not working for you?
That's weird 🤔

I'm fine with making that flow customizable though.
I would recommend doing it like this:

1. Extract the function we're currently providing to the Children.forEach into a buildChildrenLayout function that returns the children layout array.
2. Export buildChildrenLayout from the container.
3. Add a customChildrenLayoutBuilder prop to the container, default it with buildChildrenLayout, and pass it down to React.Children.forEach.

In this way you can customize the children layout flow while still allowing to re-use the default behaviour, if needed 👍
Does it make sense to you? ANy feedback/alternative?

[Nantris]

Thanks for your reply! I think that makes sense!

The only this that doesn't make sense to me is that adding displayName didn't have the desired effect. I don't see any problems with the logic - but the components I passed would always end up in otherChildrens anyway.

The best solution, in my view, would be if this worked as it apparently should. I wondered if it might somehow be related to the usage of React.FC - because I just have no other theories. No ideas about what might be going wrong there? It's especially weird, because it obviously works for the default components - I just can't replicate that behavior in custom components.

From an earlier comment:

I try setting .name and .displayName, naming the function itself to match, but nothing seems to work.

Very strange!

[Nantris]

It looks like this code would work. The only changes here are to add .displayName to the end of child.type and to the end of each case ... line

What do you think about this approach? Would you accept a PR for it? It would keep the API simpler than the other approaches we discussed.

React.Children.forEach(children, (child) => {
    if (typeof child === "object" && child !== null && "type" in child) {
      switch (child.type.displayName) {
        case DialogTitle.displayName:
          titleChildrens.push(child as TitleElement);
          return;
        case DialogDescription.displayName:
          descriptionChildrens.push(child as DescriptionElement);
          return;
        case DialogButton.displayName:
          if (Platform.OS === "ios" && buttonChildrens.length > 0) {
            buttonChildrens.push(
              <View
                style={[
                  verticalButtons
                    ? styles.buttonSeparatorVertical
                    : styles.buttonSeparatorHorizontal,
                  buttonSeparatorStyle,
                ]}
              />
            );
          }
          buttonChildrens.push(child as ButtonElement);
          return;
      }
    }
    otherChildrens.push(child);
  });

---

That would resolve the primary issue we were facing. It would leave the issue of custom TextInputs unaddressed though: #141 (comment)

In our case we have a TextInput component like this that we'd like to use to avoid having to duplicate the styling for the DialogInput to look like the input we already have. It would be great to just pass this in and have it all just work.

export const TextInput = forwardRef((props, ref) => {
  const theme = useTheme();
  const { text, selection } = theme.activeMode();
  return (
    <StyledTextInput
      placeholderTextColor={getPlaceholderColor(text)}
      ref={ref}
      style={props.style}
      underlineColorAndroid={props.underlineColorAndroid || 'transparent'}
      selectionColor={selection.text}
      {...props}
    />
  );
});

[mmazzarolo]

It looks like this code would work. The only changes here are to add .displayName to the end of child.type and to the end of each case ... line

What do you think about this approach? Would you accept a PR for it? It would keep the API simpler than the other approaches we discussed. Still not sure how

Generally, I'm not a fan of merging things without knowing why/how they work...
That said, I think I just noticed the issue: we're comparing the component type as a strict reference, which won't work for your own custom components. Meaning we're basically doing this: child.type === DialogTitle. Where DialogTitle is react-native-dialog's component, so it will never be true for your own custom component.
So, I'm OK with your suggestion 👍

[mmazzarolo]

And thanks so much for keeping up with the discussion 👍
Regarding the TextInput issue, I would prefer fixing the color styling directly within react-native-dialog (following #105, I guess) -- otherwise any tiny change we'll do to the TextInput component might become a breaking change if we allow customizing it that way. If you're in hurry for the DialogInput styling, you can either pass down the entire component (copy-pasting it) or point your react-native-modal-dialog version to #142 (with something like yarn add <GitHub user name>/<GitHub repository name>#<branch/commit/tag>)

[Nantris]

@mmazzarolo thanks for your knowledge in this discussion and all the hard work you've done. I've submitted a PR for the .displayName change here: #143.

I understand the desire not to complicate things with allowing custom components to be passed. I think what I'll likely end up doing is forking the repo and adding our own changes to support a custom DialogInput there so we can easily keep up with this repo as the upstream - and this way anyone else can use that fork as well if needed.

Unfortunately that would still leave the labelStyle unaddressable without further modifications. As far as I can recall labelStyle not being customizable was the only thing that kept us from being able to implement a dark mode completely on our own, without any changes to the react-native-dialog source code.

Otherwise end users can style everything else properly, but labels remain untargetable and end up as black text on black background. I know you don't want to allow for any more styling/customization than is strictly necessary, but supporting that until/unless there's a way to force isDark to work as intended would make the library a lot more flexible than one would assume of such a small change. I think it's worthwhile and in the vein of completing what I feel is a missing piece of the existing styling API you've provided.

Thanks again for the great work!

[mmazzarolo]

@slapbox after thinking about it a bit more, I agree with your point. I think it's fine to "temporarily" support the customization of stuff we can't automatically customize yet.
I'm OK with making the label and switch style customizable, as long as 1) we clearly state in the README that it's just a temporary workaround until we can fix the dark theming and 2) we name them something like customLabelStyleMightBreakInTheFuture.
Would you be open to take care of creating such PR?

[Nantris]

Would you be open to take care of creating such PR?

Yup, will do! It might be a few days or even a bit longer before I can get it done, but it's on my list along with fixing #143.

What do you think of the naming convention, unstableLabelStyle or unstable_labelStyle?

[mmazzarolo]

I'm fine either way 👍

[Nantris]

This can be closed once #150 is reviewed and merged.