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

Incorrect label with attachments count #5460

Closed
sosnovsky opened this issue Oct 18, 2023 · 6 comments · Fixed by #5472
Closed

Incorrect label with attachments count #5460

sosnovsky opened this issue Oct 18, 2023 · 6 comments · Fixed by #5472

Comments

@sosnovsky
Copy link
Collaborator

When message has attached public key and other attachment, UI shows 2 Attachments label, while it should be 1 Attachment as public key attachment isn't shown:

Screenshot 2023-10-18 at 22 59 36

For example you can check PDF attached thread in flowcrypt.compatibility account

@martgil
Copy link
Collaborator

martgil commented Oct 30, 2023

@sosnovsky In the discussion on #5459, it has been decided to hide the label completely, irrespective of the number. Can we hide the label completely, as reverting the previous change seems to be functioning effectively and would require less effort.

If you agree on this, it would be part of #5472 PR as it was really a small change -- i'll just reference this GH issue on the commit if thats ok with you?

@sosnovsky
Copy link
Collaborator Author

@sosnovsky In the discussion on #5459, it has been decided to hide the label completely, irrespective of the number. Can we hide the label completely, as reverting the previous change seems to be functioning effectively and would require less effort.

I checked our discussion there, but didn't find any comment about hiding attachments count label. There we discussed only hiding some noname attachments, but attachments count label should be visible.

@martgil
Copy link
Collaborator

martgil commented Oct 31, 2023

Sorry, I mean the initial description in the GH issue. So, I'm little bit confused as to whether hide it or not:

After merging #5445 noname attachments are not hidden in password-protected messages and 2 attachments label is visible, but it should be hidden too.

It could be superseded by #5460 -- do you want me to display this label for both pgp encrypted and password protected message?

@sosnovsky
Copy link
Collaborator Author

Sorry, I mean the initial description in the GH issue. So, I'm little bit confused as to whether hide it or not:

Ah, there I meant that 2 attachments label should be hidden when there is no rendered attachments.

It could be superseded by #5460 -- do you want me to display this label for both pgp encrypted and password protected message?

Yes, attachments count label should be visible for any message when there is at least 1 visible attachment.

In this task description there is a message with 2 attachments, but one of them is public key attachment which is not rendered. So attachments count label should be visible and have text 1 Attachment.

In #5459 message has 2 attachments, but both of them are hidden, so attachments count label should be hidden too.

@martgil
Copy link
Collaborator

martgil commented Oct 31, 2023

Oh, okay. But in the current code, we only show the label if there are two or more renderable attachments. I just to confirm you would like it to be changed as suggested.

@sosnovsky
Copy link
Collaborator Author

Oh, okay. But in the current code, we only show the label if there are two or more renderable attachments. I just to confirm you would like it to be changed as suggested.

Comment there says Aligned with Gmail, the label is shown only if there are 2 or more attachments, but currently Gmail shows label even when there is only 1 attachment. So let's remove nRenderedAttachments >= 2 check and show this label when there is at least 1 renderable attachment, to align with Gmail behavior

sosnovsky pushed a commit that referenced this issue Nov 16, 2023
* add conditional statement for hiding noname attachment

* wip: update test

* wip: add test for password messages

* #5460 hide number of attachments

* #5460 add live gmail test

* wip: live gmail test

* wip: fix incorrect attachment count

* pr reviews: fix incorrect number of attachments

* bug fix: hide attachment container only when .pgp_pubkey is hidden

* pr reviews: include missing test on gmail.ts

* wip: revert changes on semaphore.yml

* wip: trim blank line

* pr reviews: fix bug

* wip: live-gmail-test

* wip: revert changes on semaphore.yml

* pr reviews: fix minor issues

* wip: fix test for filename

* wip: fix ui for pgp block button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants