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

Delete button on asset view page #12940

Merged
merged 19 commits into from
Jul 25, 2023
Merged

Conversation

akemidx
Copy link
Collaborator

@akemidx akemidx commented Apr 27, 2023

Description

A button has been added to the Asset detail view page that allows for deletion (archiving) of that asset.

Additionally, the dropdown menu in the upper right has been removed and replaced with buttons on the right column.
Screenshot 2023-04-27 at 7 12 50 PM

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested Locally.

Checklist:

@what-the-diff
Copy link

what-the-diff bot commented Apr 27, 2023

PR Summary

  • Fixes typo in AssetsController.php
    A typo issue has been resolved in the AssetsController.php file

  • Adds delete confirmation to view asset page
    A confirmation prompt has been added before deleting an asset on the view asset page

  • Updates language file for assets/general
    The language file for assets/general has been updated with a new "delete_confirm" string and missing translations have been added

  • Improves buttons layout on view asset page
    Buttons on the view asset page have been rearranged for better access and visibility, added checkin button and improved restore functionality

  • Code cleanup and fixes
    Removed unused code, fixed typos, and improved code structure by adding comments for increased clarity

@akemidx
Copy link
Collaborator Author

akemidx commented Apr 27, 2023

There is a placeholder comment on the hardware/view blade for a generic image if no image is provided by the user

@@ -146,9 +146,9 @@ function () {
[AssetFilesController::class, 'show']
)->name('show/assetfile');

Route::delete('{assetId}/showfile/{fileId}/delete',
Copy link
Owner

Choose a reason for hiding this comment

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

This will break file deletion on assets. We already have a delete route - the one we use for table-button deletes. No route changes should be required here, unless I'm missing something (and I might be)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can double check we still use this. With the rewrite it probably isn't used. but I can look at it this weekend.

Copy link
Owner

Choose a reason for hiding this comment

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

We don't work on weekends :) It will keep until monday :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that was old, and couldn't get it to work. but it should be gone off the final bit

Copy link
Owner

Choose a reason for hiding this comment

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

I'm still showing this route as deleted in the diff?

@akemidx
Copy link
Collaborator Author

akemidx commented Apr 28, 2023

we can chat Monday. i think the merge i did messed some stuff up. I also just need to commit less i guess.

@@ -91,7 +91,6 @@
'debug_warning' => 'Warning!',
'debug_warning_text' => 'This application is running in production mode with debugging enabled. This can expose sensitive data if your application is accessible to the outside world. Disable debug mode by setting the <code>APP_DEBUG</code> value in your <code>.env</code> file to <code>false</code>.',
'delete' => 'Delete',
'delete_confirm' => 'Are you sure you wish to delete :item?',
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I thought I added this back in??

I originally thought we didn't need it cause you said we had a string already, but this was probably the strong you were talking about.

)->name('delete/assetfile');
Route::delete('{assetId}/delete',
[AssetsController::class, 'destroy']
)->name('hardware/delete');
Copy link
Owner

Choose a reason for hiding this comment

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

This route already exists - we shouldn't have to add or remove anything from routes for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, it was removed on my side and I had added it back. Sorry got crossed up.

@snipe snipe merged commit a01cb26 into snipe:develop Jul 25, 2023
@akemidx akemidx deleted the delete_asset_from_view_page branch April 23, 2024 19:03
@akemidx akemidx restored the delete_asset_from_view_page branch April 23, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants