Skip to content

Conversation

Nantris
Copy link
Contributor

@Nantris Nantris commented May 10, 2022

Overview

Add an additional style prop to the Dialog.Input

Test Plan

Why: 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. Basically everything else seems to be possible with the current API, but changing the label color is not currently possible.

@Nantris
Copy link
Contributor Author

Nantris commented May 10, 2022

@mmazzarolo would you be open to accepting this PR? Otherwise we'll have to fork the library, but my goal today was to abandon our old fork of the library to upgrade to the new version which supports dark/light theme natively.


export interface DialogInputProps extends TextInputProps {
label?: ReactNode;
labelStyle?: StyleProp;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure of proper value here

@mmazzarolo
Copy link
Owner

Hey @slapbox ! Thanks for this PR.
Just to clarify, is this style not applied correctly? It should be automatically applied based on the native settings. Let me know!

@Nantris
Copy link
Contributor Author

Nantris commented May 11, 2022

It's working based on the native settings, but there's no way to override light mode to force dark themes besides passing in custom styling, right?

Our goal is to allow a dark dialog when our app is in dark mode, even if the system-level preference is set to light mode.

@mmazzarolo
Copy link
Owner

@slapbox I understand. Yeah, you're right. Question: would it be enough for your use case to be able to "force" one of the two styles (dark/light) with a prop instead of supplying your own custom style? I'm asking this because I'd like to keep the customization options to the minimum.

@mmazzarolo
Copy link
Owner

mmazzarolo commented May 12, 2022

Ha! Just noticed you suggested something similar here: #141 (comment) :)
I would make it more explicit, with something like: theme: "light" | "dark" | "auto" = "auto".

@Nantris
Copy link
Contributor Author

Nantris commented May 12, 2022

would it be enough for your use case to be able to "force" one of the two styles (dark/light) with a prop instead of supplying your own custom style?

I think yes, but I tried implementing that yesterday by modifying the component files to take an isDark prop and modifying useTheme.js to accept that as an argument, but was surprised to find it had no effect on Android. Is PlatformColor able to get colors for the non-active mode? In other words, if the system is set to light mode and we forced the dialog to render in dark mode, can PlatformColor get the correct colors for us on Android? If so, I'm not sure why my experiment from yesterday failed.

@mmazzarolo
Copy link
Owner

In other words, if the system is set to light mode and we forced the dialog to render in dark mode, can PlatformColor get the correct colors for us on Android? If so, I'm not sure why my experiment from yesterday failed.

Oh. I thought it should be able to get that value regardless of the current theme, but I'm not sure :/

@Nantris
Copy link
Contributor Author

Nantris commented May 13, 2022

Any thoughts on these changes I tried making? Did I overlook something?

Nantris@418f08f

I'm doing more research into this myself also.

@mmazzarolo
Copy link
Owner

@slapbox , wondering if this is related to #105 .
Next week I'll try to setup my machine to work on RN (right now I'm missing everything -- sdks, etc)

@Nantris
Copy link
Contributor Author

Nantris commented May 18, 2022

I managed to get a single dark mode color working in light mode in an Expo Snack, which gives me at least a starting point and some proof that you can access light/dark colors outside of their respective modes. But I can't get them working in the app, or even any colors to work in this example: https://github.com/facebook/react-native/blob/main/packages/rn-tester/js/examples/PlatformColor/PlatformColorExample.js

Quite strange, but some minor progress.

@mmazzarolo
Copy link
Owner

Thanks for the update @slapbox 🙏

@Nantris
Copy link
Contributor Author

Nantris commented Jun 9, 2022

I think this would also need to be implemented in Switch.tsx if accepted, to maintain consistency. It would be even better if we could get isDark forced properly though. @mmazzarolo have you had any time to setup a React Native development environment? I'm curious if you're able to get the dark colors working when the system is in light mode, or vice versa.

@mmazzarolo
Copy link
Owner

mmazzarolo commented Jun 9, 2022

@slapbox copy-paste from the other thread:

@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?

(or use this PR)

@Nantris
Copy link
Contributor Author

Nantris commented Jun 9, 2022

I'll close this PR and submit a new one soon along the lines of our discussion in #141.

@Nantris Nantris closed this Jun 9, 2022
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.

2 participants