Skip to content

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

Merged
peter-fogg merged 3 commits into
masterfrom
peter-fogg/fix-upload-confirmation
Jul 1, 2013
Merged

Show a confirmation on asset delete. Fixes STUD-375.#302
peter-fogg merged 3 commits into
masterfrom
peter-fogg/fix-upload-confirmation

Conversation

@peter-fogg

Copy link
Copy Markdown
Contributor

No description provided.

@cahrens

cahrens commented Jun 28, 2013

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

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

What's the build failure from? Violations?

@cahrens

cahrens commented Jun 30, 2013

Copy link
Copy Markdown

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
Copy Markdown
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

cahrens commented Jul 1, 2013

Copy link
Copy Markdown

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

cahrens commented Jul 1, 2013

Copy link
Copy Markdown

👍

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