Page MenuHomePhabricator

[native] Use new Alert wrapper
ClosedPublic

Authored by patryk on Jul 28 2023, 3:24 AM.
Tags
None
Referenced Files
F2196251: D8648.id29159.diff
Fri, Jul 5, 12:12 PM
Unknown Object (File)
Wed, Jul 3, 10:23 PM
Unknown Object (File)
Tue, Jul 2, 6:19 PM
Unknown Object (File)
Tue, Jul 2, 8:12 AM
Unknown Object (File)
Mon, Jul 1, 2:21 AM
Unknown Object (File)
Sat, Jun 29, 1:08 PM
Unknown Object (File)
Fri, Jun 21, 10:27 PM
Unknown Object (File)
Tue, Jun 18, 10:17 PM
Subscribers

Details

Summary

Solution for ENG-4421.
This diff replaces all native Alert occurrences with a new Alert wrapper, which sets the currently selected theme as an Alert theme.

Depends on D8586.

Test Plan

Test on iOS (setting alert theme is only supported on this platform) and on Android (to check if nothing breaks).
Prior to testing, make sure what theme you have set on iOS device. Then set app theme opposite to theme set on device. This will make sure, that Alerts use app theme rather than system theme.

Testing each location where the Alert import was replaced can be time-intensive, therefore only selected areas have been inspected to determine if the Alert function operates as expected:

  • Login panel -> invalid username, empty password
  • Edit message -> cancel editing
  • Edit password -> not matching passwords, incorrect pasword
  • Logout -> keep login info in chain alert
  • Delete account panel -> invalid password
  • Chat -> leave chat

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

patryk held this revision as a draft.
patryk edited the test plan for this revision. (Show Details)
patryk added reviewers: bartek, tomek, inka.

Please note that the only place where we still use Alert, imported from react-native, is in redux-setup.js. utils/alert.js uses the redux store to determine which theme to use. Attempting to use the alert wrapper prior to store initialization results in a You may not call store.getState() while the reducer is executing error. Although we could bypass this issue by using store.subscribe(variableUpdateCallback), the following reasons underscore why it's not an optimal approach:

  1. The variable updates with every redux store update, potentially leading to performance issues.
  2. We face a require cycle problem.

You can emulate second reason by changing import in redux-setup.js to our alert wrapper and replacing getCurrentTheme in utils/alert.js with something like this:

// ...
let currentTheme = defaultGlobalThemeInfo.activeTheme;
store.subscribe(() => {
  const nextTheme =
    store.getState().globalThemeInfo.activeTheme ||
    defaultGlobalThemeInfo.activeTheme;
  if (nextTheme !== currentTheme) {
    currentTheme = nextTheme;
  }
});
// Remember to swap all `getCurrentTheme()` occurrences to `currentTheme`
// ...

One additional thing to check might be Android - even though the theme doesn't apply there, it is worth checking if nothing breaks.

This revision is now accepted and ready to land.Jul 28 2023, 6:09 AM

Please note that the only place where we still use Alert, imported from react-native, is in redux-setup.js. utils/alert.js uses the redux store to determine which theme to use. Attempting to use the alert wrapper prior to store initialization results in a You may not call store.getState() while the reducer is executing error. Although we could bypass this issue by using store.subscribe(variableUpdateCallback), the following reasons underscore why it's not an optimal approach:

  1. The variable updates with every redux store update, potentially leading to performance issues.
  2. We face a require cycle problem.

You can emulate second reason by changing import in redux-setup.js to our alert wrapper and replacing getCurrentTheme in utils/alert.js with something like this:

// ...
let currentTheme = defaultGlobalThemeInfo.activeTheme;
store.subscribe(() => {
  const nextTheme =
    store.getState().globalThemeInfo.activeTheme ||
    defaultGlobalThemeInfo.activeTheme;
  if (nextTheme !== currentTheme) {
    currentTheme = nextTheme;
  }
});
// Remember to swap all `getCurrentTheme()` occurrences to `currentTheme`
// ...

Great explanation of your reasoning!!

patryk edited the test plan for this revision. (Show Details)

Rebase and replace Alert in role-utils.react.js

This revision was landed with ongoing or failed builds.Aug 4 2023, 12:46 AM
This revision was automatically updated to reflect the committed changes.