Skip to content

Routes - convert form alerts to toast notifications for creating, editing and deleting #1567

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 5, 2017
Merged

Routes - convert form alerts to toast notifications for creating, editing and deleting #1567

merged 2 commits into from
Jun 5, 2017

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented May 22, 2017 •
edited by spadgett
Loading

https://trello.com/c/6WVJdgjL

@spadgett ptal

Task

  • Modify js to enable persistent error/warning notification to auto dismiss upon successful form submission.

@spadgett spadgett self-requested a review May 22, 2017 19:48
@spadgett spadgett self-assigned this May 22, 2017
spadgett
spadgett requested changes May 22, 2017
View reviewed changes
app/scripts/controllers/createRoute.js Outdated
@@ -17,6 +17,7 @@ angular.module('openshiftConsole')
AuthorizationService,
DataService,
Navigate,
NotificationsService,
ProjectsService,
keyValueEditorUtils) {
$scope.alerts = {};
Copy link
Member

Choose a reason for hiding this comment

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

You can remove $scope.alerts since it's no longer used and also remove the alerts element from create-route.html

app/scripts/controllers/edit/route.js Outdated
@@ -17,6 +17,7 @@ angular.module('openshiftConsole')
AuthorizationService,
DataService,
Navigate,
NotificationsService,
ProjectsService,
RoutesService) {
$scope.alerts = {};
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here: remove $scope.alerts and remove <alerts> from the view

@spadgett
Copy link
Member

Modify js to enable persistent error/warning notification to auto dismiss upon successful form submission.

Let's open a separate issue so we don't forget if we don't do it in this PR. @jwforres FYI

@jwforres
Copy link
Member

If we open a separate issue for that it needs to be fixed by stagecut. I don't want to have us in a state in online where you submit a successful form and have to manually close old errors.

@spadgett
Copy link
Member

@jwforres We might hold off on this PR in that case. It requires changes to NotificationsService in origin-web-common. But I do think we could fix the AlertsMessageService cases now, which won't have this problem.

@sg00dwin sg00dwin mentioned this pull request May 22, 2017
Closed
@spadgett
Copy link
Member

@sg00dwin @jwforres Maybe I can knock the common change for this out quickly.

@jwforres
Copy link
Member

jwforres commented May 22, 2017 via email

@jwforres
Copy link
Member

jwforres commented May 22, 2017 via email

@sg00dwin sg00dwin force-pushed the route-conversion-toast branch from 828a97d to ba5c3e6 Compare May 22, 2017 20:35
@spadgett spadgett mentioned this pull request May 22, 2017
Merged
@spadgett
Copy link
Member

I opened a PR that adds remove and truncates long notification text:

openshift/origin-web-common#76

@spadgett
Copy link
Member

@sg00dwin I opened a PR against your branch with the changes, but you'll need the updates I made to common.

sg00dwin#3

@spadgett spadgett changed the title Routes - convert form alerts to toast notifications for creating, editing and deleting [WIP] Routes - convert form alerts to toast notifications for creating, editing and deleting May 23, 2017
@spadgett
Copy link
Member

Added [WIP] since this is waiting on a origin-web-common release now.

@sg00dwin sg00dwin force-pushed the route-conversion-toast branch 2 times, most recently from e737fab to 13a3b68 Compare May 24, 2017 19:52
@sg00dwin sg00dwin mentioned this pull request May 25, 2017
Closed
@sg00dwin sg00dwin force-pushed the route-conversion-toast branch from 13a3b68 to 852af7c Compare June 5, 2017 18:01
@spadgett spadgett changed the title [WIP] Routes - convert form alerts to toast notifications for creating, editing and deleting Routes - convert form alerts to toast notifications for creating, editing and deleting Jun 5, 2017
@spadgett
Copy link
Member

spadgett commented Jun 5, 2017

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 852af7c

@openshift-bot
Copy link

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link

Evaluated for origin web console test up to 852af7c

@openshift-bot
Copy link

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1453/) (Base Commit: b70413b)

1 similar comment
@openshift-bot
Copy link

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1453/) (Base Commit: b70413b)

@openshift-bot
Copy link

openshift-bot commented Jun 5, 2017 •
edited
Loading

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1459/) (Base Commit: 3427432)

@openshift-bot openshift-bot merged commit eef932f into openshift:master Jun 5, 2017
@sg00dwin sg00dwin deleted the route-conversion-toast branch June 6, 2017 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

Assignees

@spadgett spadgett

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sg00dwin @spadgett @jwforres @openshift-bot