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

Show a confirmation on asset delete. Fixes STUD-375. #302

Merged
merged 3 commits into from
Jul 1, 2013

Conversation

peter-fogg
Copy link
Contributor

No description provided.

@cahrens
Copy link

cahrens commented Jun 28, 2013

The code itself looks fine. But I'm worried about how it looks when you delete multiple assets. It's not useful after the first deletion, and confusing if you cancel a deletion.

@peter-fogg
Copy link
Contributor Author

Talked to Brian about this -- the confirmation shows up at the bottom of the page now, and disappears after two seconds.

@chrisndodge
Copy link
Contributor

I haven't tried the branch, but the code looks good.

What's the build failure from? Violations?

@cahrens
Copy link

cahrens commented Jun 30, 2013

I like this much, much better. We should consider changing Advanced Settings as well (and Course Settings?).

I ran the Lettuce test and it passed. I was worried about timing since the delete notification now dismisses, but it looks like the CSS is still present once it has been shown, even if enough time has passed for it to auto-hide?

Can't view Jenkins failures from home.

@peter-fogg
Copy link
Contributor Author

The tests (unit tests and Lettuce) all run locally for me -- I can't see Jenkins errors as I don't have a kerberos login. It was probably a pylint error (fixed in the latest commit).

@cahrens
Copy link

cahrens commented Jul 1, 2013

It was actually test failures. But they would appear to be completely unrelated, so let's see what happens when your last commit runs.

@cahrens
Copy link

cahrens commented Jul 1, 2013

👍

peter-fogg pushed a commit that referenced this pull request Jul 1, 2013
Show a confirmation on asset delete. Fixes STUD-375.
@peter-fogg peter-fogg merged commit 388644b into master Jul 1, 2013
@peter-fogg peter-fogg deleted the peter-fogg/fix-upload-confirmation branch July 1, 2013 14:33
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
fix courseware css file so it imports the right files in the right order
mattdrayer added a commit that referenced this pull request Nov 4, 2014
…nsitional_ids

mattdrayer/api-migrate_transitional_ids: Cleaned up two additional table...
yokose-ks added a commit to nttks/edx-platform that referenced this pull request Oct 2, 2015
CrewS pushed a commit to CrewS/edx-platform-1 that referenced this pull request Feb 18, 2019
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
* Commits:
  Remove Travis-CI integration
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