Issue#616 #916
Issue#616 #916
Conversation
| <div class="modal-content"> | ||
| <div class="modal-header"> | ||
| <button type="button" class="close" data-dismiss="modal">×</button> | ||
| <h3 class="modal-title">{{_ "apiBackends_Delete_API_Title"}}</h3> |
There was a problem hiding this comment.
Rename this i18n token to deleteApiBackendConfirmation_header and add corresponding translation string to en.i18n.js.
|
Try not to use session variables much. There are usually ways to do things in the local scope. E.g. template instance variables and reactive variables. Reference
|
|
All i18n tokens should be unique and prefixed with the template name. We are not re-using translations, as per the guidance of @baijat. |
| Meteor.call('removeApiBackend', apiBackendId); | ||
|
|
||
| $('#confirmDelete').hide(function() { | ||
| $('#successDelete').removeClass('hide'); |
There was a problem hiding this comment.
Consider using the s-alert package to display the success message. That way we don't have to manually handle showing and hiding the success dom element. S-alert is already installed in our project.
|
Overall, great work here! This is a big feature to tackle. Please review the above suggestions, and let me know if I may be of assistance. |
|
@brylie I made almost all the changes you wanted, only thing was I was not sure about how to include |
|
@brylie Another point was with s-alert - when deleting an API from the API details page (not the manage API page), the s-alert was not appearing when the deletion was successful and the user had been returned to the API list page (catalogue). I don't know if this is related to how fast the catalogue page is loading. |
|
@Alapan, the s-alert is not high priority. It won't block this code from being merged. We can just open an enhancement, after finishing this task :-) |
| } else if (currentRoute === 'manageApiBackends') { | ||
| Router.go('manageApiBackends'); | ||
| } | ||
| sAlert.success(instance.backendName + " was successfully deleted!"); |
There was a problem hiding this comment.
Move this sAlert.success() call into one of the if, else if, or else blocks.
|
Try moving the sAlert call into the |
| // based on name of current route, load suitable parent page | ||
| var currentRoute = Router.current().route.getName(); | ||
|
|
||
| if (currentRoute === 'viewApiBackend') { |
There was a problem hiding this comment.
Rewrite this block using a switch. Using switch will improve readability and possibly reduce the probability of semantic errors, as seen below.
There was a problem hiding this comment.
I thought about using switch-case earlier, but I wasn't sure what to use for the default case.
There was a problem hiding this comment.
You can use whatever is in your else statement. I.e. else is similar to default. Otherwise, consider just using break as the default, when no default action is desired.
There was a problem hiding this comment.
Right now, I don't have anything for else, as I am not sure if I can set any default route. I can replace the else if with a separate if statement, though.
|
@brylie I used switch-case and moved the s-alert inside each case, but the result was the same as before. I tried |
| Meteor.call('deleteApiBackendOnApiUmbrella', apiUmbrellaApiId, function(error, apiUmbrellaWebResponse) { | ||
|
|
||
| if (apiUmbrellaWebResponse.http_status === 204) { | ||
|
|
There was a problem hiding this comment.
As was pointed out in review, make sure to publish the changes on API Umbrella. This is a design of API Umbrella that changes do not take effect, at the nginx level, until they are published. We have some example code you can review.
There was a problem hiding this comment.
@brylie I wrote the code for publishing in my last commit. I hope I did it correctly. Please review.
|
@Alapan, this looks great. I went ahead and fixed the Bootstrap button group markup, so that it renders correctly. Please review and merge this PR, if everything looks good. |
|
@brylie Merged. |
Link - https://github.com/apinf/api-umbrella-dashboard/issues/616