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 multiple file/directory deletion in the filemanager #544

Merged
merged 15 commits into from Jul 20, 2017

Conversation

OrangeJuiced
Copy link
Contributor

@OrangeJuiced OrangeJuiced commented Jul 10, 2017

This pull request contains the code for multiple file/directory deletion in the file manager. I added a selection button per item in file manager. The user can delete the selected items using the delete button in the top right.

It looks like this:

@@ -19,6 +19,9 @@
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
var files = [];
Copy link
Member

Choose a reason for hiding this comment

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

These are ES6 based files, please use let or const rather than var.

Copy link
Member

Choose a reason for hiding this comment

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

This name is also rather unclear, maybe use selectedFiles.

swal({
type: 'warning',
title: '',
text: 'Are you sure you want to delete <code>' +
Copy link
Member

Choose a reason for hiding this comment

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

I think this would render better if each file being deleted was wrapped in <code> tags, rather than all wrapped together in a single tag.

}, () => {
$.ajax({
type: 'DELETE',
url: `${Pterodactyl.node.scheme}://${Pterodactyl.node.fqdn}:${Pterodactyl.node.daemonListen}/server/file/f/${files}`,
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't have a different endpoint for mass deletions to pass data as a JSON object, rather than a URL argument.

@@ -85,7 +88,7 @@
{{ $carbon->diffForHumans() }}
@endif
</td>
<td><button class="btn btn-xxs btn-default disable-menu-hide" data-action="toggleMenu" style="padding:2px 6px 0px;"><i class="fa fa-ellipsis-h disable-menu-hide"></i></button></td>
<td><button class="btn btn-xxs btn-default disable-menu-hide" data-action="toggleMenu" style="padding:2px 6px 0px;"><i class="fa fa-ellipsis-h disable-menu-hide"></i></button><button class="btn btn-xxs btn-default disable-menu-hide" data-action="addToList" style="padding:2px 6px 0px;"><i class="fa fa-plus disable-menu-hide"></i></button></td>
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking we should make these checkboxes and have a mass-options dropdown menu that for now only contains delete, but could down the road handle moving and such.

Copy link
Member

Choose a reason for hiding this comment

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

If you do that, you don't have to manage selections in javascript as well, just find all the selected checkboxes.

@DaneEveritt DaneEveritt added the feature request A request for a new feature. label Jul 10, 2017
@DaneEveritt DaneEveritt added this to the v0.7.0 milestone Jul 10, 2017
@BentHaase
Copy link
Contributor

BentHaase commented Jul 10, 2017

selection_893

I would rather suggest an approach seen in the image where the checkboxes are either beneath the items or to the left of the items with one checkbox on top that allows to select all.

This sample image is taken from Nextcloud

@@ -19,6 +19,9 @@
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
var files = [];
var fileselements = [];
Copy link
Member

Choose a reason for hiding this comment

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

this should be camelCase aka filesElements

@@ -85,7 +88,7 @@
{{ $carbon->diffForHumans() }}
@endif
</td>
<td><button class="btn btn-xxs btn-default disable-menu-hide" data-action="toggleMenu" style="padding:2px 6px 0px;"><i class="fa fa-ellipsis-h disable-menu-hide"></i></button></td>
<td><button class="btn btn-xxs btn-default disable-menu-hide" data-action="toggleMenu" style="padding:2px 6px 0px;"><i class="fa fa-ellipsis-h disable-menu-hide"></i></button><button class="btn btn-xxs btn-default disable-menu-hide" data-action="addToList" style="padding:2px 6px 0px;"><i class="fa fa-plus disable-menu-hide"></i></button></td>
Copy link
Member

Choose a reason for hiding this comment

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

If you do that, you don't have to manage selections in javascript as well, just find all the selected checkboxes.

@@ -19,6 +19,9 @@
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
var files = [];
Copy link
Member

Choose a reason for hiding this comment

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

This name is also rather unclear, maybe use selectedFiles.

@BentHaase
Copy link
Contributor

@schrej can you post a screenshot of that in action in your dev setup?

@schrej
Copy link
Member

schrej commented Jul 11, 2017

@ET-Bent just get yourself some vagrant ;)

@OrangeJuiced
Copy link
Contributor Author

OrangeJuiced commented Jul 11, 2017

The feature should now be more in line with the suggestions.

It now looks like this:



@DaneEveritt
Copy link
Member

Looks great! Can we make it so left clicking a row selects the file checkbox?

Copy link
Member

@schrej schrej left a comment

Choose a reason for hiding this comment

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

Looks good from my side now.

@schrej
Copy link
Member

schrej commented Jul 13, 2017

Actually, is the mass actions button disabled when no file is selected? If not, why not?

@OrangeJuiced
Copy link
Contributor Author

That should do it. Both suggestions are implemented.

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

Successfully merging this pull request may close these issues.

None yet

4 participants