Skip to content

Conversation

@arthurdenner
Copy link
Contributor

What kind of change does this PR introduce?

Bug fix. Closes #3077.

What is the current behavior?

I noticed a few issues on the Storage Management modal:

  1. If you select two files, unselect one of them and press to delete, we send the id for both files because we are not filtering which files have { [id]: true };
  2. If you try to delete a file outside the editor, it fails because we try to access state.editor.currentSandbox.owned without checking for currentSandbox first;
  3. If you open the modal outside the editor, the buttons to add files to the project are enabled.

What is the new behavior?

  1. We are only sending the current selected files;
  2. We are checking for currentSandbox before trying to read currentSandbox.owned;
  3. We are disabling the button and tooltips for adding files to the project when the modal is opened outside the editor.

What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.

Delete files flow

  1. Open the modal outside the editor
  2. Select files and press to delete them
  3. Verify that the files are deleted.

Add files flow

  1. Open the modal outside the editor
  2. Verify that the buttons for adding files are disabled.

Checklist

  • Documentation
  • Testing
  • Ready to be merged
  • Added myself to contributors table

Notes

I'd rather remove the buttons for adding files when the modal is opened outside the editor, but didn't took this route because it would required more changes and I'd like to get your feedback.

@lbogdan lbogdan temporarily deployed to pr3081 November 25, 2019 12:08 Inactive
@lbogdan
Copy link
Contributor

lbogdan commented Nov 25, 2019

Build for latest commit d91fc83 is at https://pr3081.build.csb.dev/s/new.

Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

This is really good, thanks for the fix. I like how you decide with the function whether it's possible, this is a much better UX. I left one code style comment, once that's done we can merge this.

Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

Great! Thanks for this fix!

@CompuIves CompuIves merged commit 2230824 into codesandbox:master Nov 25, 2019
@arthurdenner arthurdenner deleted the fix/storage-management branch November 25, 2019 13:15
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.

Can't delete files in storage management

3 participants