Solution for ENG-3935.
This diff introduces a wrapper for React Native alerts, which synchronizes currently selected theme in the app with alert style. Right now userInterfaceStyle is only supported on iOS.
Light theme alert:
Dark theme alert:
Details
Details
Dark theme alert:
- Select a location where you want to implement an alert (for example, you can add useEffect in root.react.js).
- Verify whether the alert theme is consistent with the app theme.
- Navigate to profile > appearance, alter the theme, and check if the same transformation applies to the other theme.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/utils/alert.js | ||
|---|---|---|
| 8–11 ↗ | (On Diff #28885) | We need to store information about options argument position, according to method name. Maybe there exists a better way to avoid hardcoding positions (ex. by using reflection) but alert API probably won't change in the future. |
| 35 ↗ | (On Diff #28885) | Using typeof, because Alert uses static methods, which are unavailable on Alert type. |
| native/utils/alert.js | ||
|---|---|---|
| 35 ↗ | (On Diff #28885) | We could also implement this wrapper by creating a new class with static method called alert, but flow wants a type for method arguments. We could do it, but we would need to use Parameters<F> (which is available from 0.209). |
Comment Actions
[deleted] My bad, turns out if you don't reload the app manually those changes don't take effect for some reason
| native/utils/alert.js | ||
|---|---|---|
| 26–28 ↗ | (On Diff #28885) | Why if there is no userInterfaceStyle, you delete other option fields?? |
Comment Actions
I don't like the solution with hardcoding argument positions, but I tried to figure out a clearly better way, and couldn't find one. There are many that are about equally bad in my opinion. But I would prefer if @tomek or someone else could take look as well.
Comment Actions
Is there a diff where we start using this instead of the default Alert?
I don't like the solution with hardcoding argument positions, but I tried to figure out a clearly better way, and couldn't find one. There are many that are about equally bad in my opinion. But I would prefer if @tomek or someone else could take look as well.
Agree that this isn't ideal. Wondering how type-safe this solution is. Also, not sure if accessing state directly won't cause any issues (e.g. if we store Alert.alert in a variable, would it update after state change?).
How about creating a custom hook, e.g. useAlert() that returns { alert, prompt } that have the same signatures as the original ones, but have userInterfaceStyle hardcoded? In this hook we can useSelector so that the functions get updated after state change.
| native/utils/alert.js | ||
|---|---|---|
| 24 ↗ | (On Diff #28928) | Is it safe to specify this for Android? |
Comment Actions
How about creating a custom hook
Creating a custom hook is not a viable option. Consider dev-menu.js as an example. There are instances where we use Alert outside of a React component.
Also, not sure if accessing state directly won't cause any issues (e.g. if we store Alert.alert in a variable, would it update after state change?).
I've tried storing Alert.alert in a variable and didn't notice any problems. We could potentially use a method like store.subscribe to update the variable when the state changes. However, after reviewing the example in the documentation, it seems this might not be necessary -- we would still use the store.getState() method. Also using store.subscribe would be inefficient, because we would update the variable even when we don't use alert.
I'm going to refactor the code: rather than focusing on creating the "generic code", we should focus on being type-safe (e.g. when alert arguments change, flow should throw an error).
| native/utils/alert.js | ||
|---|---|---|
| 24 ↗ | (On Diff #28928) | |
| native/utils/alert.js | ||
|---|---|---|
| 9–10 ↗ | (On Diff #28987) | We're exporting this type, so it should be read-only to make sure that consumers don't modify it |