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

MINOR Open file details after the last file has been uploaded #808

Merged

Conversation

lukereative
Copy link
Contributor

Fixes #489

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

The WYSIWYG part seems to be working fine. However looks like there's been some regression on the File management UI.

If I upload a file via the file management interface and delete it right away, it's still visible until I refresh the page (the behat failure is legit in this case).

If I check back the 1 branch, it works fine.

@@ -309,26 +309,6 @@ describe('Gallery', () => {
expect(props.actions.queuedFiles.removeQueuedFile).not.toBeCalled();
});

it('should openFile if type is "insert-media"', () => {
Copy link
Contributor

@maxime-rainville maxime-rainville Jul 15, 2018

Choose a reason for hiding this comment

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

Why are those tests no longer relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we decided that it should open the uploaded file on both views (asset-admin and insert media) so we no longer check for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Just wanted to make sure.

@lukereative
Copy link
Contributor Author

lukereative commented Jul 16, 2018

@maxime-rainville Turns out the failing test wasn't actually related to my change (it was failing locally for me on 1 branch as well), the test was incorrectly clicking the delete button while uploading which had the same selector as the checkbox element it was trying click, I made the step more specific so it only clicks the checkbox not any action that the file has (not sure why it's suddenly become an issue). However I did modify the behaviour so the details panel doesn't open if you have other files checked and you upload.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I pull your changes down, and I'm still seeing the same behavior. https://youtu.be/c8ZVXdeHKLY

But after playing with the 1 branch, I can see it has a similar problem. The only difference is that you can delete the file correctly if you don't select the detail view after upload. https://youtu.be/jhjSh3qZOw4

I'm happy to merge it as-is and raise a separate issue

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I'm still seeing the broken behavior on the PR branch, but after further inspection, I can see a similar behavior on the 1 branch as well.

The only difference is that on the 1 branch, the file can be deleted immediately after upload as long as you don't look at the details of the file.

I'm OK merging this as-is and raise a separate issue if you don't think it's worth fixing it now.

@maxime-rainville
Copy link
Contributor

Sorry I'm repeating myself :-P

@lukereative
Copy link
Contributor Author

I think that should be fixed as a separate issue, I think there's some todo's in the code and maybe some issues in the backlog that I've seen regarding updating newly uploaded files that might be relevant #781 #782

@maxime-rainville maxime-rainville merged commit 79f8b4f into silverstripe:1 Jul 16, 2018
@maxime-rainville maxime-rainville deleted the pulls/1/open-successame branch July 16, 2018 23:10
@maxime-rainville
Copy link
Contributor

I've created #809 about the ghost entry.

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.

2 participants