UI - Border radius of attached protected pdf does not match with parent component

[kbecciv]

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. Go to staging.new.expensify.com
2. Go to any conversation
3. Send password protected pdf
4. Notice blue border of protect attachment
5. Notice there is white space in all corner

Expected Result: Blue border radius of protected pdf should match with parent component

Actual Result:

Blue border radius of protected pdf does not match with parent component (leaves white spaces in corner)

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.2.11.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @dhairyasenjaliya

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664363401205419

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @johncschuster (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[melvin-bot[bot]]

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

[Justicea83]

This can be external

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[melvin-bot[bot]]

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

[ajaynagwani777]

PROPOSAL

if (html !== text) {
                const editedTag = props.fragment.isEdited ? '<edited></edited>' : '';
                const htmlContent = html + editedTag;
                return (
                    <RenderHTML
                        html={props.source === 'email'
                            ? `<email-comment>${htmlContent}</email-comment>`
                            : `<comment>${htmlContent}</comment>`}
                    />
                );
            }

The issue is with the HTML being rendered. If we set the border-color to white in the HTML it will be fixed. Check the image below:

@kbecciv

[eVoloshchak]

@ajaynagwani777 I think we still need that grey outline, but with the correct border-radius. Setting border-color to white seems more like a workaround, since we're just disabling the outline essentially

[ajaynagwani777]

@eVoloshchak

I gave the solution because the gray outline is not visible apart from the incorrect border. Still if we need the border to be there we can set the border-radius to 15px , rn it is 8px in HTML.

[eVoloshchak]

I gave the solution because the gray outline is not visible apart from the incorrect border.

For me it's visible

Still if we need the border to be there we can set the border-radius to 15px , rn it is 8px in HTML.

Yeah, that would be perfect! Could you please include a code snipped with this change?

[ajaynagwani777]

@eVoloshchak

In styles.js -> tagStyles -> img Existing object:

img: {
            borderColor: themeColors.border,
            borderRadius: variables.componentBorderRadiusNormal, // 8
            borderWidth: 1,
        }

Updated object:

img: {
            borderColor: themeColors.border,
            borderRadius: 15,
            borderWidth: 1,
        }

[eVoloshchak]

That's what it looks like for me with borderRadius: 10. We need to figure out why it's inconsistent, so in case we change the border radius of this card (or blue border around it), this issue doens't appear once again. They should have the same variable with border radius

[ajaynagwani777]

That's what it looks like for me with borderRadius: 10. We need to figure out why it's inconsistent, so in case we change the border radius of this card (or blue border around it), this issue doens't appear once again. They should have the same variable with border radius

Sorry, it was 15. I mentioned 10 by mistake. Also, we will keep the border variable. I have mentioned the value here just for reference.

We need to figure out why it's inconsistent, so in case we change the border radius of this card (or blue border around it), this issue doens't appear once again. They should have the same variable with border radius

The blue border is of the image. By doing the borderRadius: 15, we can resolve the issue.

[Puneet-here]

Proposal

I think the image that is being used should be fixed so that it fits correctly but if we want a front end solution I propose changing the border radius but only for the pdf

we can check if it's a pdf by Str.isPDF(source)

and if it's true we will use borderRadius 15 ( we will add it in the styles.js) https://github.com/Expensify/App/blob/93587b19296f6c07567b5576418725ae213321ed/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js#L73

 style={[styles.webViewStyles.tagStyles.img, Str.isPDF(source) && styles.bR15]}

[eVoloshchak]

I think the image that is being used should be fixed so that it fits correctly

Agree, I also think this is the best solution. It would make it consistent with all of the other images

but if we want a front-end solution I propose changing the border-radius but only for the pdf, we can check if it's a pdf by Str.isPDF(source)

Awesome, that's what was missing from the previous proposal. I think it should be 14 instead of 15, but it's kinda hard to tell

To summarize, we have 2 ways to fix this issue

1. Replace the "Password protected PDF" image with the one with the correct border-radius (probably requires changes to the back end)
2. Change border-radius on the front-end for the "Password protected PDF" image only

@marcaaron, could you please take a look at this, what do you think?

[marcaaron]

I think the image that is being used should be fixed so that it fits correctly

Agree with this approach as well, but I would maybe modify it so that the image can have any custom border set in the front end so that we do not need to cut a new image each time someone changes the border.

So basically something like:

• generic version of this image without a border at all
• if we have the pdf that is password protected put the blue border on it
• otherwise use the grey one

Where does this image come from? I'm not really sure tbh. Maybe @Expensify/design knows.

[shawnborton]

Ah yeah, we should just use a version that has no rounded borders, I agree.

Hmm I'm not sure where it's hosted, but I remember @michelle-thompson working on it not that long ago? Maybe she can dig up the GH for it?

[michelle-thompson]

I found the graphic but have to remove the border first - @shawnborton do you think we should make any updates to font/branding while we're at it?

[michelle-thompson]

Since the illustration will change and thus will stick out quite a bit, let's keep it the same for now. File without border can be found here: https://www.dropbox.com/s/19vxy82ho1wltzn/EXP_password-protected.png?dl=0

[shawnborton]

That makes sense to me. Do you have the old GH handy from where you first did this update? I'm trying to figure out where this is hosted - it might be something that we need to update internally on the Expensify end in the Web repo.

[melvin-bot[bot]]

@eVoloshchak, @marcaaron Huh... This is 4 days overdue. Who can take care of this?

[michelle-thompson]

I wasn't able to find it in my issues, but I think I worked with @thienlnam on this before so he might know!

[thienlnam]

Yes, agree on all the sentiments here that we should be updating the image asset instead of making a front-end solution.

Here's that old issue for adding this - https://github.com/Expensify/Expensify/issues/136829

[michelle-thompson]

@thienlnam do you know if the image is in the Web repo somewhere?

[thienlnam]

It's in the Web-PDFs repo var/pdfs.expensify.com/assets/encrypted-file-thumbnail.jpg

https://github.com/Expensify/Web-PDFs/blob/2240636796dd4ab730329552341bea83227efb5d/var/pdfs.expensify.com/assets/encrypted-file-thumbnail.jpg

[marcaaron]

Going OOO for a week so unassigning, but looks like this is going to be internal before it's external - so I think we should consider just making it internal.

[shawnborton]

Agree, I think we need to assign internally to an engineer to update the file in the location Jack posted above.

[melvin-bot[bot]]

@michelle-thompson Whoops! This issue is 2 days overdue. Let's get this updated quick!

[shawnborton]

I can take this one now. I think this actually needs to be assigned to an internal engineer though, so let me try to do that, unless you can quickly tackle this one @thienlnam?

[thienlnam]

I'm heading OOO after today as well, though if it's just to update the file locations you should have write access to make a PR in that location (maybe?). But I'll just pullerbear another engineer to help ya out with this

[thienlnam]

Alright, nevermind auto-assigner hates me - actually IIRC when engineers add the engineering label it doesn't assign anyone so you might have to do it @shawnborton

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

[melvin-bot[bot]]

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

[thienlnam]

Oh goodness sorry for the pings, I accidentally pressed the external label but it assigned an engineer so problem failed successfully

[sketchydroide]

the new file is a png and the old one a jpg, is this a problem, should they both be the same formart? I'm not sure we use the formart when when ussing assets. @shawnborton

[shawnborton]

Here is an updated version: password-protected-thumbnail.jpg.zip

[sketchydroide]

It seems like Web-PDFs is not deployable at the moment, we need to wait for the upgrade to php8

[shawnborton]

All good, thanks for the update!

[JmillsExpensify]

Just noticed that no BZ is assigned to this issue so assigning myself per our new process.

[sketchydroide]

BZ?

[marcaaron]

Check this @sketchydroide -> https://docs.google.com/document/d/1JVk4S8r_ets5lwM6KdpggQHG7li_H3VSE8qETYbe6gI/edit

[sketchydroide]

ahh Bug Zero of course

[JmillsExpensify]

Huh, I'm not certain what's going on in this issue. The linked PR is merged, but is Web-PDFs still not deployable?

[sketchydroide]

last time I checked it was because we couldn't deploy Web-PDFs yet due to the PHP 8 thing, I'll check if this has changed in the last week

[JmillsExpensify]

Cleared up in Slack. Wed-PDFs is indeed waiting on the PHP 8.1 upgrade. Let's keep it a weekly for now. This should resolve itself by next week.

[sketchydroide]

@iwiznia any update on the PHP upgrade and deploying this? 🙇🏼 Sorry to bring you in, but it's just easier to track that way

[iwiznia]

Yes, I am working on it. I am close to finishing the PRs and will try to put them in staging tomorrow hopefully.

[JmillsExpensify]

Oh right, I have been meaning to bug Ionatan on this one. 😄

[JmillsExpensify]

Looks like this issue is just waiting on a production deploy to close.