Skip to content

[Enhancement] Model upload and delete UI#375

Merged
birm merged 3 commits intocamicroscope:developfrom
mgautam98:patch-201
Apr 16, 2020
Merged

[Enhancement] Model upload and delete UI#375
birm merged 3 commits intocamicroscope:developfrom
mgautam98:patch-201

Conversation

@mgautam98
Copy link
Copy Markdown
Contributor

@mgautam98 mgautam98 commented Apr 16, 2020

Description

After uploading model the form will close and a popup will show that the model is uploaded successfully. Previously, user was directed to manually refresh.

After deleting a model a popup for confirmation is shown to the user instead of alert("Deleted"). Also initialized the UI after deletion.

Deleted popup.css from apps/viewer it is not used anywhere instead css/popup.css is used

Motivation and Context

**Why is this change required? What problem does it solve?

  • To improve user experience

Screenshots (if appropriate):

Types of changes

**What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

**Go over all the following points, and put an x in all the boxes that apply.
**(If you're unsure about any of these, don't hesitate to ask. We're here to help!)

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@birm birm self-requested a review April 16, 2020 20:51
Copy link
Copy Markdown
Member

@birm birm 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 to me. Liking the additional usage of the toast-style popups. :)

@birm birm merged commit f393feb into camicroscope:develop Apr 16, 2020
if (status) {
alert('Deleted', name);
showInfo();
modelName.splice(modelName.indexOf(name.split('/').pop().split('_').splice(2).join('_').slice(0, -3)), 1);
Copy link
Copy Markdown
Contributor

@leoarc leoarc Apr 17, 2020

Choose a reason for hiding this comment

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

Hey @mgautam98 . I had added the line 748 to remove the model name from modelName array so that the name could be reused once that model has been deleted. Is it not required ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This line is not needed anymore. After deletion I am calling initUIComponent which will fill the modelName array again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh sorry for the confusion. I missed that . Thanks for clearing that for me!


async function deleteModel(name) {
if (confirm('Are you sure you want to delete this model?')) {
modelName = name.split('/').pop().split('_').splice(2).join('_').slice(0, -3);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

modelName is the global array which stores the names of the model for the checkDuplicateNameFunction . I think you should use a different variable name in line 743 or else the array is reinitialized wrongly every time deleteModel is called . Am I missing something @mgautam98 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. A different name should have been used I missed it. It will not cause any problem because after deleting the modelName will be reinitialized so no a problem. But It is confusing and I will correct it in next PR or you can do it too if you want in you next PR.

@birm birm mentioned this pull request Apr 17, 2020
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.

3 participants