fix: Long filename handling in Preview by mananjadhav · Pull Request #7097 · Expensify/App

mananjadhav · 2022-01-08T17:42:09Z

Details

Long filename was overflowing outside the container. Added a component that adds ellipses (...) in the middle of the string.
Updated subtitle prop to Header so that a custom component or string can be passed to Header

Fixed Issues

$ #6873

Tests

Tested with long filenames on Desktop, Web, and Mobile Apps
Tested Fluid width to ensure ellipses show only if there isn't space for the whole name to show up.
Tested ReportingTypingIndicator as the component was updated.

Verify that no errors appear in the JS console

QA Steps

Launch the app and open any chat
Upload an attachment with a very long name
Open the attachment with the preview pane
Ensure that the filename should be visible with ... and ensure that the extension should show up.

[] Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

src/components/Header.js

src/components/TextWithEllipses/index.js

src/components/AttachmentModal.js

mananjadhav · 2022-01-08T18:55:36Z

@parasharrajat PR updated with 1 pending clarification and 1-2 questions.

src/components/AttachmentModal.js

mananjadhav · 2022-01-08T20:09:21Z

PR updated.

parasharrajat

The issue MWeb.

mananjadhav · 2022-01-09T14:38:55Z

The issue MWeb.

I am not able to reproduce. Whats the filename and the device? I am trying on iPhone Safari.

parasharrajat · 2022-01-09T14:40:40Z

this is Android M web.

file name => bored-cat asds sdsds asrrrrrrr asd sad asas sdas a rajat asdsa sad as asdaasdbaccxbcbxcbxbcbxsads asdjaskdjaskdjsakdjaskdjsadsadsad.gif

mananjadhav · 2022-01-09T15:34:47Z

Got it. Well, this issue exists with ReportingTypingIndicator as well currently.

Every time I've used this on the web, I also use white-space: nowrap, but wasn't sure if I should add right now. Adding it shows like this:

Do you have any other suggestions?

parasharrajat · 2022-01-09T17:04:53Z

Oh, Ok. If this issue exists on the main as well, I would report separately but now as we are working on a change that replaces both at once so we will have to fix it. We can't merge with a known bug.

let's see if we can find a solution. But if you say that we should tackle this separately, then let me know. I am happy to report it.

parasharrajat · 2022-01-09T17:13:18Z

But I see why this is caused. and it's not your Code's fault. It's an issue with RN-web and I am reporting it there and There is another issue #6913 due to that bug.

parasharrajat · 2022-01-09T17:17:15Z

Here is the RN-web issue link necolas/react-native-web#2186

src/styles/styles.js

…splay

…display

mananjadhav · 2022-01-09T19:03:37Z

@parasharrajat PR updated

but now as we are working on a change that replaces both at once so we will have to fix it. We can't merge with a known bug.

Agreed and hence I've fixed it with a solution albeit not fixing the root cause. It seems the ellipsis isn't working well with the multi-line styles but I haven't come across this earlier.

you say that we should tackle this separately, then let me know. I am happy to report it.

While I've tackled the issue and if it is accepted, can we add a bonus to this job to solve the other bug as well?

parasharrajat · 2022-01-10T04:55:04Z

Ok, I have found the fix for the RN-web issue and I am going to submit a PR that will fix three issues at once in our repo including the one which you faced.

let's wait for that PR to merge.

mananjadhav · 2022-01-10T14:16:46Z

@parasharrajat Should I revert my recent changes? or wait for your PR to get merged? I saw your PR its the ideal fix for the issue.

parasharrajat · 2022-01-10T14:18:16Z

Le's wait for today. Thanks.

mananjadhav · 2022-01-13T13:36:48Z

@parasharrajat I can see the RNWeb PR is still not merged. Are you sure that this will fix the issue then I can probably revert my recent changes and close it from my side?

parasharrajat · 2022-01-13T14:26:21Z

Let's give it a couple of more days. I think it will fix it. I will create a tracking issue in the meantime.

parasharrajat · 2022-01-15T02:27:29Z

@mananjadhav You can revert the changes for the ellipsis fix. PR is #7254. Then we can merge this.

…le-preview-line # Conflicts: # src/styles/styles.js

mananjadhav · 2022-01-18T23:10:48Z

@parasharrajat @Julesssss PR updated.

parasharrajat · 2022-01-19T12:54:06Z

src/components/AttachmentModal.js

        this.submitAndClose = this.submitAndClose.bind(this);
        this.closeConfirmModal = this.closeConfirmModal.bind(this);
        this.isValidSize = this.isValidSize.bind(this);
+        this.splitExtensionFromFileName = this.splitExtensionFromFileName.bind(this);


No extra bindings.

parasharrajat

LGTM, tests well.

cc: @Julesssss
🎀 👀 🎀 C+ reviewed

Julesssss

Nice work. Looks good to me.

I was going to suggest a var name change, but it is too minor to care about for now.

OSBotify · 2022-01-20T12:47:40Z

✋ 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-01-21T01:00:11Z

🚀 Deployed to staging by @Julesssss in version: 1.1.31-2 🚀

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

OSBotify · 2022-01-25T19:47:11Z

🚀 Deployed to production by @AndrewGable in version: 1.1.32-0 🚀

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

mananjadhav added 6 commits January 6, 2022 23:45

fix: Added component for Text with Ellipses

bd1965c

fix: Replaced existing reporting typing logic with new component

c3a9b29

fix: Added subtitle component for Header

89b1902

fix: Place subtitle component with filename

8837961

fix: Added subtitle text styling

ce094d1

fix: fixed proptypes and moved function to component

86902b8

mananjadhav requested a review from a team as a code owner January 8, 2022 17:42

MelvinBot requested review from Julesssss and parasharrajat and removed request for a team January 8, 2022 17:42

fix: Code cleanup

8a1820c

parasharrajat suggested changes Jan 8, 2022

View reviewed changes

mananjadhav added 5 commits January 8, 2022 23:56

fix: Renamed component to TextWithEllipsis

b8249f6

fix: Removed extra styling blocks and updated comments

5c828cd

fix: Updated proptype for react comp

7552b88

fix: Code cleanup and update variable naames

36b033f

fix: Moved to style array handling to utils

5bc21a9

parasharrajat suggested changes Jan 8, 2022

View reviewed changes

src/components/AttachmentModal.js Outdated Show resolved Hide resolved

src/components/AttachmentModal.js Outdated Show resolved Hide resolved

src/components/AttachmentModal.js Outdated Show resolved Hide resolved

src/components/AttachmentModal.js Outdated Show resolved Hide resolved

fix: Trim original filename and removed trim from props

6e48f8e

mananjadhav requested a review from parasharrajat January 8, 2022 19:46

fix: Changed width to max-width

db65a55

parasharrajat suggested changes Jan 9, 2022

View reviewed changes

fix: Added space-wrap handling

15b47e3

parasharrajat suggested changes Jan 9, 2022

View reviewed changes

src/styles/styles.js Outdated Show resolved Hide resolved

mananjadhav added 2 commits January 10, 2022 00:27

fix: Changed Text with TextWithOverflow component to handle inline di…

042b7eb

…splay

fix: Added TextWtihOverflow component to handle ellipsis with inline …

20f897c

…display

mananjadhav requested a review from parasharrajat January 9, 2022 19:03

mallenexpensify mentioned this pull request Jan 12, 2022

[HOLD for payment 2022-02-01] File name isn't fully visible in Preview page #6873

Closed

This was referenced Jan 13, 2022

mweb Safari - Chat - Download icon is overlapping with text in Preview #7204

Closed

[Tracking] Single line Text is not clipping correctly #7206

Closed

mananjadhav added 3 commits January 19, 2022 03:52

fix: Revert TextWithOverflow component changes

2ca4803

Merge branch 'main' of https://github.com/mananjadhav/App into fix/fi…

57d8ebb

…le-preview-line # Conflicts: # src/styles/styles.js

fix: Removed ellipses extra styles

2d6065f

parasharrajat suggested changes Jan 19, 2022

View reviewed changes

fix: Remove extra bindings

aa9e480

parasharrajat approved these changes Jan 19, 2022

View reviewed changes

Julesssss assigned mananjadhav Jan 20, 2022

Julesssss approved these changes Jan 20, 2022

View reviewed changes

Julesssss merged commit 439116a into Expensify:main Jan 20, 2022

OSBotify mentioned this pull request Jan 20, 2022

Deploy Checklist: New Expensify 2022-01-20 #7347

Closed

33 tasks

Conversation

mananjadhav commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mananjadhav commented Jan 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mananjadhav commented Jan 8, 2022

Uh oh!

parasharrajat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mananjadhav commented Jan 9, 2022

Uh oh!

parasharrajat commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mananjadhav commented Jan 9, 2022

Uh oh!

parasharrajat commented Jan 9, 2022

Uh oh!

parasharrajat commented Jan 9, 2022

Uh oh!

parasharrajat commented Jan 9, 2022

Uh oh!

Uh oh!

mananjadhav commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parasharrajat commented Jan 10, 2022

Uh oh!

mananjadhav commented Jan 10, 2022

Uh oh!

parasharrajat commented Jan 10, 2022

Uh oh!

mananjadhav commented Jan 13, 2022

Uh oh!

parasharrajat commented Jan 13, 2022

Uh oh!

parasharrajat commented Jan 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mananjadhav commented Jan 18, 2022

Uh oh!

parasharrajat Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Jan 20, 2022

Uh oh!

mananjadhav commented Jan 8, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat commented Jan 9, 2022 •

edited

Loading

mananjadhav commented Jan 9, 2022 •

edited

Loading

parasharrajat commented Jan 15, 2022 •

edited

Loading