Page MenuHomePhabricator

[native] Trigger NodeHeightMeasurer when font scale changes
ClosedPublic

Authored by bartek on Oct 25 2022, 3:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 11:26 PM
Unknown Object (File)
Sat, Nov 2, 11:26 PM
Unknown Object (File)
Sat, Nov 2, 11:26 PM
Unknown Object (File)
Sat, Nov 2, 11:25 PM
Unknown Object (File)
Sat, Nov 2, 11:24 PM
Unknown Object (File)
Thu, Oct 24, 1:50 AM
Unknown Object (File)
Wed, Oct 23, 11:47 AM
Unknown Object (File)
Wed, Oct 23, 11:46 AM

Details

Summary

Resolves ENG-1632. This diff addresses the scenario when a user changes font scale in system settings while the Comm app is in background. In this case, when returned to the app, the message heights measured by NodeHeightMeasurer were wrong.

I solved this by creating an app state listener which is triggered when the app comes from background to foreground, and then it compares the current system font scale with the last known value. If they're different, it triggers a complete measurement.

NodeHeightMeasurer utilizes various React component lifecycle methods to do its job. According to this diagram and the current component logic, the best way to trigger full measurement is to reuse its constructor code and recreate its "initial" state from props. This is why I extracted this code to a separate static method and used it in both the constructor and event listener.

Side note: I noticed that the NodeHeightMeasurer is a very complicated component that has very little to no comments on how it works, making it difficult to understand. I added a few minimal comments to make at least my part more understandable.

Test Plan

Tested in iOS Simulator by debugging the component lifecycle (console.log works too) while doing the following steps:

  1. opening the app
  2. swiping it to the background, opening Settings -> Accessibility -> Screen and text size -> Bigger text -> sliding the slider at the bottom
  3. going back to the app and observing the console

Note that for the first time, the log warnings about height inconsistencies can still occur, but then the component is re-rendered again and these warnings disappear. This happens because one re-render can happen even before the new measurement completes. This should not be an issue though.

Diff Detail

Repository
rCOMM Comm
Branch
@barthap/remeasure-font
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek edited the test plan for this revision. (Show Details)
bartek added reviewers: tomek, ashoat, atul.
bartek published this revision for review.Oct 25 2022, 4:13 AM
ashoat added 1 blocking reviewer(s): tomek.

Looks great!! Please see inline nits before landing. Would also love if @tomek could take a look

native/components/node-height-measurer.react.js
6

Can we import this type from native/types/react-native.js instead? The React Native team often changes Flow types... by having an alias in our codebase, we make it a little easier to update our types when theirs change

207–218

Nit: I prefer to exit early (see inline edit)

This revision is now accepted and ready to land.Oct 26 2022, 1:06 AM
bartek edited the summary of this revision. (Show Details)

Apply suggestions from code review

native/components/node-height-measurer.react.js
6

Sure! Good thing to know btw.

207–218

Yeah, I missed that. Agreed, the code is much more readable this way!