-
Notifications
You must be signed in to change notification settings - Fork 27
[WIP] refactor: Separate remove button into own controller #699
Conversation
*/ | ||
vm.removePortlet = function removePortletFunction(fname) { | ||
layoutService.removeFromHome(fname).success(function() { | ||
$scope.$apply(function(request, text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure $scope.$apply
is needed here.
AFAICT All the operations all the operations are synchronous, and exception handling could be managed by the wrapping promise.
it appears be used to indirectly trigger $scope.$digest
, which can be called directly.
}); | ||
}).error( | ||
function(request, text, error) { | ||
alert('Issue deleting ' + fname + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably outside of the scope of this PR, but a toast would look a lot nicer than an alert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Evolving in-app messaging capabilities of uPortal-app-framework
is on MyUW's roadmap for this quarter, for whatever that's worth.
Hey @thevoiceofzeke, it looks like a merge conflict has cropped up. Would you be able to resolve the conflict? |
Closing this PR because I FUBAR'd it by deleting my fork that included the HEAD branch and it's easier to just redo the work in my new fork than figure out how to |
In this PR:
Why?
The functionality used to live in
LayoutController
, and every instance of a<remove-button>
in a user's layout was causingLayoutController
to initialize (including thegetLayout()
service call. It's probably better if each instance of the remove button only initializes what it needs to function.Proof that it still works
Contributor License Agreement adherence: