Remove clipped texts throughout the app when device font size is increased #10312

osama256 · 2022-08-08T20:55:35Z

Details

This PR gives you the ability to increase or decrease the device font size without seeing clipped texts throughout the app.

the App behavior will be :

when the device font size is decreased the App font size will decrease.
when the device font size is increased the App font size will increase.
when the device font size is increased and exceeded a certain larger level PixelRatio.getFontScale() * default value > max value ? max value : default value * PixelRatio.getFontScale() the App font size will not increase because any increasing above this level will show clipped texts throughout the app.
if the device font size is changed while the App is in the background, this change will not affect the App until close and reopen the App again.
this is similar to what YouTube does to handle the device font size change.

Fixed Issues

$ #7541

Tests

Increase the device font size.
Open the App.
Verify there is no Text clipping throughout the app.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Increase the device font size.
Open the App.
Verify there is no Text clipping throughout the app.

Verify that no errors appear in the JS console

Screenshots

Web

web.mov

Mobile Web

mweb.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

melvin-bot · 2022-08-08T20:55:38Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

Santhosh-Sellavel · 2022-08-10T16:06:03Z

Bug:
The New Message notification badge is shown after changing font size

Screen.Recording.2022-08-10.at.9.34.06.PM.mov

Santhosh-Sellavel

Bug has been found while testing

Signed-off-by: osama256 <51829206+osama256@users.noreply.github.com>

osama256 · 2022-08-11T13:02:04Z

Fixed.

Also, there was another bug (badge icon alignment)

Fixed here.

Santhosh-Sellavel · 2022-08-14T18:05:26Z

@osama256 Still the text 0 new message is not centered properly

Santhosh-Sellavel · 2022-08-15T19:31:31Z

@osama256 bump

osama256 · 2022-08-16T10:58:07Z

Fixed.

Santhosh-Sellavel

@pecanoro
This PR is almost done but seeking clarifications on a few things, then we should be good to merge this.

@osama256 Thanks for being patient, and helping this PR move forward. We're mostly done.
Can you answer those questions & pull the latest changes from main?

Thanks!

src/components/Button.js

Santhosh-Sellavel · 2022-08-31T15:02:44Z

Seems conflicts again @osama256

Santhosh-Sellavel · 2022-09-09T18:25:49Z

@osama256

Bug: Drop down arrow is not centered for larger text in iOS & Android

Santhosh-Sellavel · 2022-09-09T18:38:30Z

@pecanoro

This is the PR that I've been testing for a longer period, every time I am ready to approve I am running into a new bug which is the reason this PR is tested for a longer period.

I requesting that this PR should be treated as a feature rather than a bug. I believe there will be no deploy blocker, but there is the slightest chance there may be a breakage which should be treated as new bugs.

Because this PR more or less has an impact on the whole app, which require more time as more eyes notice small bugs because these bugs are really easy to miss!

Once the last found bug here is fixed let's proceed with merging this one after testing a bit.

Please share your thoughts, thanks!

Santhosh-Sellavel

#10312 (comment)

osama256 · 2022-09-15T15:56:14Z

@Santhosh-Sellavel sorry for the delay, I'm working on it.

osama256 · 2022-09-18T10:04:08Z

Fixed.

Santhosh-Sellavel · 2022-09-18T17:32:24Z

src/styles/styles.js

@@ -804,7 +804,7 @@ const styles = {
    },
    picker: (disabled = false, error = false, focused = false) => ({
        iconContainer: {
-            top: variables.pickerIconTop,
+            top: Math.round(variables.inputHeight * 0.5) - 10,


This is weird, care to explain this?

TO vertically center a child in its parent we need padding top = half parent height - half child height, in our case half input height = 26 and half icon height = 10 so we need padding top = 16, but because the input height increases when the device font size increases so instead of using a hard coded value we can make it dynamic.

This calculation, itself seems off can we move the computation variables at least?

Do you mean keeping top: variables.pickerIconTop in styles.js and in variables.js file using this pickerIconTop: (getValueUsingPixelRatio(52, 72) * 0.5) - 10?

Santhosh-Sellavel · 2022-12-02T17:32:19Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Screen.Recording.2022-12-12.at.12.26.28.AM.mov

Desktop

Screen.Recording.2022-12-12.at.12.20.43.AM.mov

iOS

Screen.Recording.2022-12-12.at.12.04.54.AM.mov

Android

Screen.Recording.2022-12-12.at.12.03.26.AM.mov

src/styles/styles.js

Santhosh-Sellavel · 2022-12-02T18:14:37Z

I find these two bugs while testing now

On Normal size, g is getting clipped here (I feel this could be not related to this but looks like well within scope too)

Main

Screen.Recording.2022-12-02.at.11.34.35.PM.mov

On this branch

Screen.Recording.2022-12-02.at.11.35.13.PM.mov

Search placeholder is not centered properly in larger sizes

Okay this issue exists for normal font size too I will report it if not reported. Report here

Santhosh-Sellavel · 2022-12-02T18:27:00Z

@pecanoro what should we do with the first issue, should we expect to solve it here?

As I see It's not related to what we are solving here, as the issue occurs only when the field has focus. So it might be irrelevant too, but I want to hear your thoughts too. Thanks!
cc: @luacmartins

pecanoro · 2022-12-02T18:38:51Z

Let me retest that, does it happen in all platforms or only iOS?

Santhosh-Sellavel · 2022-12-02T18:44:46Z

The first issue iOS only occurs on the main branch also for normal font size and the second issue is on all platforms I reported already in slack.

pecanoro · 2022-12-02T19:40:09Z

The first issue iOS only occurs on the main branch also for normal font size and the second issue is on all platforms I reported already in slack.

That happens even on the main branch, so I would say pretty unrelated. Also, I feel that is a regression of something else.

pecanoro · 2022-12-02T20:02:51Z

Ok, I am making the decision of merging since those bugs were not introduced by this PR and it's out of scope, this one is handling an increase or decrease in size. @Santhosh-Sellavel If you can complete the list with videos it would be great even though @luacmartins already did it. If we wait longer, this PR will never get merged since we tend to get conflicts.

Santhosh-Sellavel · 2022-12-02T20:05:57Z

I'll definitely do that, note story book is failing Rajat already reported in slack. Technically it would not have any effect, as this PR mainly for Native devices

OSBotify · 2022-12-02T20:07:55Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-12-13T22:41:32Z

🚀 Deployed to production by @chiragsalian in version: 1.2.38-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

parasharrajat · 2022-12-22T16:16:52Z

There are a couple of inconsistencies in this PR which seems to have caused two issues.

Incorrect Picker height.
[HOLD for payment 2023-02-03] Invite members: very little space between the lines and the highlighted lines are overlapping #13741

This PR should have been reviewed by the Design team.

normal line height is 18 in our app, not 16.

osama256 · 2022-12-22T19:27:48Z

@parasharrajat Thanks for mentioning these issues here, but it seems these issues are not produced by this PR, and here is why.

for Picker height inputHeight = 50 and it is the same value as in version 1.2.36 (two weeks ago) it was hardcoded 50, I know it was 52 in the previous versions (months ago) like in version 1.1.38 but someone changed it later.
For the overlapping issue, we didn't do this change and you can check it in my repo
For line height we have lineHeightNormal = 16 and lineHeightLarge = 18, we used lineHeightNormal in badgeText and peopleBadgeText and both were 16 in version 1.2.36 (two weeks ago)

And we used lineHeightLarge in label, textLabel, mutedTextLabel, formLabel and formHelp all of these were 18 in the previous version (1.2.36)

NikkiWines · 2022-12-23T15:36:18Z

👋 Hi all, we've identified this PR as at least part of the cause for #13336. Since this modified styles for generic components, more detailed steps than "Verify there is no Text clipping throughout the app" would've been good to prevent bugs from slipping through.

Discussion on steps to be taken to ensure these bugs don't slip through can be found here cc: @mollfpr

osama256 · 2022-12-26T14:49:18Z

Discussion on steps to be taken to ensure these bugs don't slip through can be found here

I can't open the link.

mollfpr · 2022-12-26T14:54:27Z

@osama256 Have you join #expensify-bugs channel?

osama256 · 2022-12-26T15:42:50Z

@osama256 Have you join #expensify-bugs channel?

@mollfpr No, I have sent an email to join.

pecanoro · 2022-12-26T15:46:29Z

@NikkiWines We discussed the clipped g here, I don't think it is a regression 😅 it was already on main before we merged the PR as @Santhosh-Sellavel pointed out here.

pecanoro · 2022-12-26T15:55:29Z

Though the emoji problem getting clipped on the top part could be coming from this one, but I haven't double checked.

osama256 · 2022-12-26T17:26:38Z

Though the emoji problem getting clipped on the top part could be coming from this one

@pecanoro Where is it?

osama256 added 2 commits August 8, 2022 21:27

Remove clipped texts throughout the app

3b54638

Merge branch 'Expensify:main' into remove_clipped_texts

41836ef

osama256 requested a review from a team as a code owner August 8, 2022 20:55

melvin-bot bot requested review from pecanoro and Santhosh-Sellavel and removed request for a team August 8, 2022 20:55

Santhosh-Sellavel suggested changes Aug 10, 2022

View reviewed changes

Fix notification badge TranslateY

9779f5e

Signed-off-by: osama256 <51829206+osama256@users.noreply.github.com>

osama256 requested a review from Santhosh-Sellavel August 11, 2022 13:02

Fix new message vertical alignment

771df89

Santhosh-Sellavel reviewed Aug 29, 2022

View reviewed changes

src/components/Button.js Show resolved Hide resolved

Merge branch 'Expensify:main' into remove_clipped_texts

6328d19

fetch upstream

f2c7b85

osama256 requested review from Santhosh-Sellavel and removed request for pecanoro September 1, 2022 06:54

Santhosh-Sellavel suggested changes Sep 14, 2022

View reviewed changes

Fix picker icon alignment

dc8f7ad

osama256 requested a review from Santhosh-Sellavel September 18, 2022 10:04

Santhosh-Sellavel reviewed Sep 18, 2022

View reviewed changes

pecanoro reviewed Dec 2, 2022

View reviewed changes

src/styles/styles.js Show resolved Hide resolved

pecanoro merged commit aed921f into Expensify:main Dec 2, 2022

aneequeahmad mentioned this pull request Dec 3, 2022

[HOLD #10312] Chat - Text "New Message" is out alignment in badge. #13294

Closed

francoisl mentioned this pull request Dec 8, 2022

Deploy Checklist: New Expensify 2022-12-07 #13431

Closed

72 tasks

Luke9389 mentioned this pull request Dec 13, 2022

Text & Dropdown Input Styles #12447

Merged

stephanieelliott mentioned this pull request Dec 15, 2022

[HOLD for payment 2022-12-20] [$500] Accessibility - Text is clipped throughout the app when device font size is increased - Reported by @rushatgabhane #7541

Closed

NikkiWines mentioned this pull request Dec 16, 2022

[HOLD for payment 2022-12-20] Text is clipped on iOS sign in page #13336

Closed

0xmiros mentioned this pull request Jan 13, 2023

fix long text cut in iOS single line input #14273

Merged

53 tasks

This was referenced Jan 31, 2023

Update baseTextInput line height to prevent cutoff #13389

Merged

[HOLD for payment 2023-02-03] Invite members: very little space between the lines and the highlighted lines are overlapping #13741

Closed

Beamanator mentioned this pull request Feb 2, 2023

[HOLD for payment 2023-02-06] [$2000] Text is cut off / out of the boundary if the font size of the device increased #13942

Closed

4 tasks

Remove clipped texts throughout the app when device font size is increased #10312

Remove clipped texts throughout the app when device font size is increased #10312

Conversation

osama256 commented Aug 8, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

melvin-bot bot commented Aug 8, 2022

Santhosh-Sellavel commented Aug 10, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

osama256 commented Aug 11, 2022

Santhosh-Sellavel commented Aug 14, 2022 • edited Loading

Santhosh-Sellavel commented Aug 15, 2022

osama256 commented Aug 16, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Aug 31, 2022

Santhosh-Sellavel commented Sep 9, 2022

Santhosh-Sellavel commented Sep 9, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

osama256 commented Sep 15, 2022

osama256 commented Sep 18, 2022

Santhosh-Sellavel Sep 18, 2022

Choose a reason for hiding this comment

osama256 Sep 19, 2022

Choose a reason for hiding this comment

Santhosh-Sellavel Sep 19, 2022

Choose a reason for hiding this comment

osama256 Sep 21, 2022

Choose a reason for hiding this comment

Santhosh-Sellavel commented Dec 2, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

Santhosh-Sellavel commented Dec 2, 2022 • edited Loading

On Normal size, g is getting clipped here (I feel this could be not related to this but looks like well within scope too)

Main

On this branch

Search placeholder is not centered properly in larger sizes

Santhosh-Sellavel commented Dec 2, 2022

pecanoro commented Dec 2, 2022

Santhosh-Sellavel commented Dec 2, 2022

pecanoro commented Dec 2, 2022

pecanoro commented Dec 2, 2022

Santhosh-Sellavel commented Dec 2, 2022

OSBotify commented Dec 2, 2022

OSBotify commented Dec 13, 2022

parasharrajat commented Dec 22, 2022

osama256 commented Dec 22, 2022

NikkiWines commented Dec 23, 2022 • edited Loading

osama256 commented Dec 26, 2022

mollfpr commented Dec 26, 2022

osama256 commented Dec 26, 2022

pecanoro commented Dec 26, 2022 • edited Loading

pecanoro commented Dec 26, 2022 • edited Loading

osama256 commented Dec 26, 2022

osama256 commented Aug 8, 2022 •

edited

Loading

Santhosh-Sellavel commented Aug 14, 2022 •

edited

Loading

Santhosh-Sellavel commented Dec 2, 2022 •

edited

Loading

Santhosh-Sellavel commented Dec 2, 2022 •

edited

Loading

NikkiWines commented Dec 23, 2022 •

edited

Loading

pecanoro commented Dec 26, 2022 •

edited

Loading

pecanoro commented Dec 26, 2022 •

edited

Loading