Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2021-12-15] [$250] Attachment - If file name is too long, attachment preview is overflowing the modal - Reported by: @Santhosh-Sellavel #6157

Closed
isagoico opened this issue Nov 1, 2021 · 32 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Design Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Nov 1, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Attach this file to a conversation looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooongname.zip
  2. Check the attachment preview

Expected Result:

Attachment name should not overflow the modal.

Actual Result:

Attachment name is overflowing the modal

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.12-0

Reproducible in staging?: Yes
Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation
image

Expensify/Expensify Issue URL:

Issue reported by: @Santhosh-Sellavel
Slack conversation: (In this case GH conversation) #5755 (comment)

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@isagoico isagoico changed the title Attachment - If file name is too long, attachment preview is overflowing the modal Attachment - If file name is too long, attachment preview is overflowing the modal - Reported by: @Santhosh-Sellavel Nov 1, 2021
@Luke9389 Luke9389 added the External Added to denote the issue can be worked on by a contributor label Nov 1, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@Christinadobrzyn
Copy link
Contributor

It looks like this bug was reported in this GH where a similar issue is being fixed.

I suggest we hold off on getting proposals for this until we see if this PR fixes the issue. What do you think @Luke9389?

@Santhosh-Sellavel
Copy link
Collaborator

First of all this issue was not addressed in the PR #5969

Also, we would still need to hold proposals.
Because as @chiragsalian said here

we might want to pull in design opinion for it and to decide if its ROI is worth it.

Then, based on the outcome of the discussion we should resume proposals!

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 2, 2021

Thanks, @Christinadobrzyn, good catch. Now that I read the PR comments on that, Chirag did recommend making a separate issue for this. I'll tag @Expensify/design to see how we want to chop this text. I think a ... in the middle of the text could be cool so we can still see the file extension at the end.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Nov 3, 2021

Sorry, @Santhosh-Sellavel I should have asked you if this fix was in the PR - thanks for clarifying.

Sounds like we'll wait to get thoughts from design on what the truncated attachment name should look like and then collect proposals.

@Christinadobrzyn
Copy link
Contributor

@Expensify/design when possible, can you let us know how you'd like the truncated attachment name should look in newdot?

@Luke9389 Luke9389 added the Design label Nov 5, 2021
@MelvinBot
Copy link

Triggered auto assignment to @michelle-thompson (Design), see these Stack Overflow questions for more details.

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 8, 2021

Still waiting on design for this one.

@MelvinBot MelvinBot removed the Overdue label Nov 8, 2021
@michelle-thompson
Copy link
Contributor

I think a ... in the middle of the text could be cool so we can still see the file extension at the end.

Yup, I agree with this!

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 9, 2021

OK, we're open for proposals on this one. 👍

@Christinadobrzyn
Copy link
Contributor

@botify botify removed the Daily KSv2 label Nov 9, 2021
@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 9, 2021
@kadiealexander kadiealexander changed the title Attachment - If file name is too long, attachment preview is overflowing the modal - Reported by: @Santhosh-Sellavel [$250] Attachment - If file name is too long, attachment preview is overflowing the modal - Reported by: @Santhosh-Sellavel Nov 10, 2021
@Anthonyamcm
Copy link

solution to this is easy adding flex to the text and a word wrap will make the text stay within the confined of the parent after that you can add any styling you want there is no technical changes needed only styling

@akshayasalvi
Copy link
Contributor

akshayasalvi commented Nov 10, 2021

Proposal

@Luke9389 We could've used the default <Text numberOfLines={1} ellipsizeMode='middle'>long long long long text<Text>, but it won't work on Desktop, Mobile Web and Web. It'll only work for iOS and Android.

I've got a couple of approaches to solve this problem:

  1. We split the file name and the extension into two texts, and add ellipsis to the fileName (Most recommended)
    const { fileName, extension } = splitFileNameAndExtension(props.name);
    <View>
        <Text numberOfLines={1}>{fileName}</Text>
        <Text>{extension}</Text>
    </View>

I've implemented a similar approach for Typing indicator here #5933

  1. We allow the filename to be truncated and then show the extension separately instead of the paper clip with a good icon. The way Whatsapp does, blue doc icon for Docx files, etc. (Cumbersome from design angle. Too many file formats to take care of)

  2. We implement the middle ellipsis mode but have separate implementations for Web, Desktop and Mobile Web. And the default one with Text ellipsisMode for mobile Apps.
    Similar to this: https://css-tricks.com/using-flexbox-and-text-ellipsis-together/

@Luke9389
Copy link
Contributor

Hi @akshayasalvi, thanks for the clear and detailed proposal.
I also like option number 1.
I looked at the PR you linked (#5933) and didn't notice the part where we actually split the name. Is that in the getDisplayName function?

@akshayasalvi
Copy link
Contributor

@Luke9389 Here we didn't split the username. What we did is we had User is typing... as one Text earlier. We split that into two Text components <Text numberOfLines={1}>User</Text><Text>is typing...</Text> wrapped into a View.

Hope this clarifies your question.

@Luke9389
Copy link
Contributor

OH ok I see, thanks @akshayasalvi.
So how are you thinking we should implement splitFileNameAndExtension?
Would there be a character limit that it checks for?

@akshayasalvi
Copy link
Contributor

So how are you thinking we should implement splitFileNameAndExtension? Would there be a character limit that it checks for?

No there won't be any character limit check. We won't have to do it because if the file name is shorter, then ellipses in first Text won't show up and render as if its just one single Text. For the longer filename, the first Text will show ... and the second Text, will show the extension.

Here's the basic function for splitting the name

function splitFileNameAndExtension(name) {
	const splitNames = name.split('.');
	const extension = `.${splitNames.pop()}`;
	const baseName = splitNames.join('.');
	return { extension, baseName };
}

In AttachmentView, we create a small renderFunction

const renderFileName() = {
   const { baseName, extension } = splitFileNameAndExtension(props.file.name);
   return (
       <View style={[styles.flexRow]}>
          <View style={[styles.flexShrink1]}>
             <Text style={[styles.textStrong]} numberOfLines={1}>{baseName}</Text>
          </View>
          <View style={[styles.flexShrink0]}>
             <Text style={[styles.textStrong]}>{extension}</Text>
          </View>
      </View>
   )

and replace the current line:

<Text style={[styles.textStrong, styles.flexShrink1]}>{props.file && props.file.name}</Text>

with:

{renderFileName()}

@Luke9389
Copy link
Contributor

Ok, I like this plan. I think we're getting pretty close here.
One thing I'm noticing is that we'll accidentally split the file name if the name itself has a . in it.
From what I understand, when should know the MIME type of the attachment right? I'm sure JS has the capability to do that. I'd need to look into the upload code to know more.

@warunyoud
Copy link

warunyoud commented Nov 11, 2021

I think another possible approach (and a proposal) is to solve this issue simply leveraging CSS styling. The logic will be quite simple. With this there are a total of 3 styling that needs to change for this issue.

  1. Specifying the max-width of the outer container (possibly max-width: 80%;). This will help ensure that the container does not exceed the size of the view.
  2. Changing the container display type to block (display: block;). This will already force the filename to wrap around and show up on multiple lines. However, it will affect the attachment icon, which leads to last change.
  3. Lastly, set the styling of the attachment icon to float: left;. This will adjust the icon to the appropriate location as before.

The final product will looks like the image below:
Screen Shot 2021-11-11 at 9 18 11 AM

@akshayasalvi
Copy link
Contributor

Ok, I like this plan. I think we're getting pretty close here. One thing I'm noticing is that we'll accidentally split the file name if the name itself has a . in it. From what I understand, when should know the MIME type of the attachment right? I'm sure JS has the capability to do that. I'd need to look into the upload code to know more.

Well, MIME-type is the better solution, I assumed string after the last '.' is the extension.

@Luke9389
Copy link
Contributor

OH, last . seems like it'll work. OK, @akshayasalvi, go ahead and spin up a PR! 🟢

@Christinadobrzyn
Copy link
Contributor

Hired @akshayasalvi in Upwork!

Also hired @Santhosh-Sellavel for reporting this issue - we will pay the $250 for proposing this when the job is complete.

@Amarpsp10
Copy link

Amarpsp10 commented Nov 11, 2021

An another simple approach just add the style property for breaking word

textBreak :{
     wordBreak: 'break-all'
 },

ouput look like
ss-localhost

@shawnborton
Copy link
Contributor

That makes sense to me. I also think it couldn't hurt to give this some kind of max-width.

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 15, 2021
@puneetlath
Copy link
Contributor

@Christinadobrzyn I accidentally ended the Upwork contract for this issue when closing #5755.

Can you pay @Santhosh-Sellavel via this contract when this issue gets completed? Sorry for the confusion!

@Christinadobrzyn
Copy link
Contributor

No worries! Yes, I'll make sure to pay @Santhosh-Sellavel for this contract when this job closes

@Christinadobrzyn
Copy link
Contributor

Hey @Luke9389 can you please give an update on this job? It looks like we're reviewing the PR?

@MelvinBot MelvinBot removed the Overdue label Nov 29, 2021
@Luke9389
Copy link
Contributor

Just merged the PR. We went for word wrapping after all 👍

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 8, 2021
@botify botify changed the title [$250] Attachment - If file name is too long, attachment preview is overflowing the modal - Reported by: @Santhosh-Sellavel [HOLD for payment 2021-12-15] [$250] Attachment - If file name is too long, attachment preview is overflowing the modal - Reported by: @Santhosh-Sellavel Dec 8, 2021
@MelvinBot MelvinBot removed the Overdue label Dec 8, 2021
@botify
Copy link

botify commented Dec 8, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.18-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2021-12-15. 🎊

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Dec 16, 2021

7 day production timeframe is complete!

Paid @akshayasalvi in Upwork.

@Santhosh-Sellavel - I rehired you for this job in Upwork so I can pay you for reporting this. Can you please let me know when you've accepted my request in Upwork and I'll pay you the $250 for reporting this? Thanks!

Keeping this open until we pay @Santhosh-Sellavel

@Christinadobrzyn
Copy link
Contributor

Since we paid @Santhosh-Sellavel for reporting this bug instead of job #5755

I paid @Santhosh-Sellavel for #5755 and closed it out.

I think we're all good with paying Santosh - let me know if otherwise!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Design Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests