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

Fix #317: Added multiple layer deletion functionality to action bar #333

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yashdusing
Copy link
Contributor

@yashdusing yashdusing commented Mar 17, 2018

Adding multiple layer functionality

@coveralls
Copy link

coveralls commented Mar 17, 2018

Coverage Status

Coverage remained the same at 95.442% when pulling 532bd56 on yashdusing:multiple_deletion into c394768 on Cloud-CV:master.

@Ram81
Copy link
Member

Ram81 commented Mar 18, 2018

@yashdusing can you post a screenshot of how it works?

@yashdusing
Copy link
Contributor Author

Step 1 : Load model & select the layer you want to delete. Add to the delete queue
screenshot from 2018-03-18 11-26-22
Step 2 : Repeat for the layers to be deleted
Step 3 : Empty the delete queue
screenshot from 2018-03-18 11-26-51
Output : (Deleting first 3 layers)
screenshot from 2018-03-18 11-27-04

@yashdusing
Copy link
Contributor Author

@thatbrguy @Ram81 could you please review the changes made ?

@yashdusing
Copy link
Contributor Author

Is this okay?

@thatbrguy
Copy link
Contributor

@yashdusing Hey, I didn't have time to check it out, maybe @Ram81 could.

But from the screenshots, here are my comments:

  • The delete button is placed at the right place. Great!
  • Is there some highlighter that would indicate what layers are added to the delete queue ?
  • Have you tried doing it without a delete queue? As in, delete immediately after selection.

@yashdusing
Copy link
Contributor Author

I’ll add something to highlight the selected layers

@yashdusing
Copy link
Contributor Author

Wouldn’t that be the same as the delete button ?

@thatbrguy
Copy link
Contributor

@yashdusing Hey, I tested your method, good work. I would still prefer if it was like the following:

  • Press the 'X' button
  • Click on any number of layers you want. The click itself either adds the layer to the queue, or deletes it
  • Press 'X' again when done

@yashdusing
Copy link
Contributor Author

yashdusing commented Mar 19, 2018

I’ll work on the highlighting part first if it’s possible just to make this pr complete and then implement the above . Is that okay ?

@thatbrguy
Copy link
Contributor

Your call. Take your time though, there's no rush.

@yashdusing
Copy link
Contributor Author

I thought about it for a while.
There are 2 approaches :
1: Delete mode button which when activated deletes the layer selected instantly.
2: Delete Queue Add and Empty dropbox which when layer is added to queue, highlight is added to the layer and then selected layers are deleted.
What approach do you think is appropriate?
The instant delete or highlight ?

@yashdusing
Copy link
Contributor Author

@thatbrguy @Ram81

@utsavgarg
Copy link
Contributor

@yashdusing Could you fix the PR message, you might have missed deleting the PR guidelines. As for the multiple delete function, could we have a delete option in the sidebar upon clicking which a checkbox pops next to each node, and all layers which have been checked can be deleted on the press of another button.

@yashdusing
Copy link
Contributor Author

yashdusing commented Mar 21, 2018

@utsavgarg earlier PR did the same thing (except the checkbox button) you asked. But, I had to shift to the action bar :/.

@utsavgarg
Copy link
Contributor

@yashdusing Sorry about the confusion, by the sidebar I did mean the one on the left (action bar).

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.

5 participants