JENKINS-62285 Don't use browser alert for notifications and confirmation #60
Conversation
src/main/webapp/js/addrole.js
Outdated
| alert('The role was added successfully'); | ||
| location.reload(); // refresh the page | ||
| showNotificationOK('The role was added successfully'); | ||
| //location.reload(); // refresh the page |
There was a problem hiding this comment.
we shouldn't leave commented out code in, it looks like the function handles this?
| //location.reload(); // refresh the page |
|
It would be best to name your PR with a summary description of what this PR does |
aytuncbeken
changed the title
src/main/webapp/js/managesids.js
Outdated
| } else { | ||
| alert('Unable to remove the sid.' + request.responseText); | ||
| showNotificationOK('Sid removed successfully.'); | ||
| } else {s |
There was a problem hiding this comment.
typo
| } else {s | |
| } else { |
| } | ||
|
|
||
| function showNotificationOK(message) { | ||
| showNotification(message, notificationBar.OK, true); |
There was a problem hiding this comment.
There's a small problem with this behaviour. The page only refreshes when the success notification disappears. So for about 2 seconds, the notification shows but the content is not updated. With the alerts, as soon as the user clicked OK, the page was refreshed. Would it be possible to use a modal popup here? Also note that I'm working on #58 which will completely change the UI.
There was a problem hiding this comment.
Hi, thanks for your comment. I will search for modal solution, not sure though if its possible or not. For providing similar functionality I added optional reload feature, which waits for notification.DELAY.
About the UI change, of course It is up to you. We can apply this change until the new UI is published.
@timja Do you know any examples for Modal implementation ? Thanks.
There was a problem hiding this comment.
better to not refresh the page and to do a live update, or let the user refresh it themselves really.
There was a problem hiding this comment.
I think that is topic of the UI change that @AbhyudayaSharma mentioned. Should we decline this request then ?
There was a problem hiding this comment.
Yeah, the code will likely still be useful, thanks for doing this!
|
Thanks @aytuncbeken ! This code will be useful for me when working on #58. I'm closing this PR then. |
Hi,
As a part of the UI Hackfest. I fixed the issue JENKINS-62285.
I hope this PR is acceptable for you.
You can see the details of the issue from here.
https://issues.jenkins-ci.org/browse/JENKINS-62285