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

Add delete option to attachments #3804

Merged
merged 5 commits into from
Jun 29, 2021
Merged

Add delete option to attachments #3804

merged 5 commits into from
Jun 29, 2021

Conversation

rdjuric
Copy link
Contributor

@rdjuric rdjuric commented Jun 29, 2021

Details

We change our shouldShow condition for the delete option to be a new function canDeleteReportAction. It is similar to our canEditReportAction, but don't check if the message is an attachment.

Fixed Issues

#3775

Tests

  1. Open any chat
  2. Send an attachment
  3. The delete option shows on the menu
  4. Clicking on the delete option correctly deletes the attachment

QA Steps

  1. Open any chat
  2. Send an attachment
  3. The delete option shows on the menu
  4. Clicking on the delete option correctly deletes the attachment

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web.mov

Mobile Web

mWeb.mp4

Desktop

desktop.mov

iOS

iOS.mp4

Android

android.mp4

@rdjuric rdjuric requested a review from a team as a code owner June 29, 2021 19:51
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team June 29, 2021 19:52
@rdjuric
Copy link
Contributor Author

rdjuric commented Jun 29, 2021

Just a note: while #3767 isn't fixed, we have to use the context menu from the side of the message to test this on the web/desktop.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Only minor NAB changes.

src/libs/reportUtils.js Outdated Show resolved Hide resolved
src/libs/reportUtils.js Outdated Show resolved Hide resolved
src/libs/reportUtils.js Outdated Show resolved Hide resolved
rdjuric and others added 3 commits June 29, 2021 19:51
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
@rdjuric
Copy link
Contributor Author

rdjuric commented Jun 29, 2021

Updated! Thanks for the corrections @luacmartins

@luacmartins luacmartins self-requested a review June 29, 2021 23:18
@luacmartins luacmartins merged commit a9db527 into Expensify:main Jun 29, 2021
@OSBotify
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants