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

[RFR] Delete view: back to previous state, not edit or list view #538

Merged
merged 3 commits into from
Jul 24, 2015

Conversation

vasiakorobkin
Copy link

If I press Cancel in delete view I want to go back where I came from, not to the deleted object's edit view (the delete button may be pressed in list view or in referenced_list field of other entity, not always in edit view; also list view or edit view or both may be disabled for deleted entity - than we would have an error). Same thing with after-delete-redirect.
I think it is much more logical to just redirect user to the previous entry in history, than to specifically edit view or list view. Is there some problems with this approach? I don't see any.

@@ -3,7 +3,7 @@
define(function () {
'use strict';

var DeleteController = function ($scope, $state, WriteQueries, notification, params, view, entry) {
var DeleteController = function ($scope, $window, $state, WriteQueries, notification, params, view, entry) {
Copy link
Member

Choose a reason for hiding this comment

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

$state isn't used anymore. Please remove it entirely from the controller.

@fzaninotto
Copy link
Member

I remember thinking about it, too, but I think this creates problems in e2e tests. As protractor failed to run, I relaunched the build, let's see.

@vasiakorobkin
Copy link
Author

Local version of Protractor reported no failures now.

@@ -43,23 +40,19 @@ define(function () {
};

DeleteController.prototype.back = function () {
var $state = this.$state;
var $window = this.$window;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need of a temp variable here. Just do a this.$window.history.back().

@jpetitcolas
Copy link
Contributor

This PR needs rebase. :)

@jpetitcolas jpetitcolas changed the title Delete view: back to previous state, not edit or list view [RFR] Delete view: back to previous state, not edit or list view Jul 2, 2015
@fzaninotto
Copy link
Member

Bump, can you rebase and refactor the back() method ?

@vasiakorobkin
Copy link
Author

Rebased branch and git push -f'ed it to vasiakorobkin/ng-admin.

One test (filter button should reappear once an unpinned filter is removed) is failing if browser window, opened by Selenium is too small. May this be a reason for Travis CI fails?

@jpetitcolas
Copy link
Contributor

Tests on master are passing. Can you try to rebase to check if it fixes your PR?

@vasiakorobkin
Copy link
Author

Rebased. All tests has been passed successfully.

jpetitcolas added a commit that referenced this pull request Jul 24, 2015
[RFR] Delete view: back to previous state, not edit or list view
@jpetitcolas jpetitcolas merged commit 5fb8036 into marmelab:master Jul 24, 2015
entity: entityName,
id: this.entityId
}, $state.params));
this.back();

Choose a reason for hiding this comment

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

looks to me like this causes a regression: if you click "delete" while on the showView page, the new code sends you "back" to that page which 404's.

Copy link
Member

Choose a reason for hiding this comment

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

@ahgittin this is taken care of in #642. Could you test it and report your findings in the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahgittin: incoming #642 indeed fixes this issue. If you are on the show view and click delete, the controller is going to check if the previous page is related to the deleted entity. If so, it redirects you to the list view. Otherwise, to the previous page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants