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

implementation of deleting from settings using modal #4438

Merged
merged 1 commit into from Sep 3, 2015

Conversation

gruiz17
Copy link
Contributor

@gruiz17 gruiz17 commented Aug 31, 2015

remove delete project modal html

several changes

  • don't show alert for just any project in the query string
  • UI fixes on modal
  • rename functions for clarity

go bin

@@ -73,6 +73,17 @@ angular.module('openshiftConsole')
DataService.list("projects", $scope, function(projects) {
$scope.projects = projects.by("metadata.name");
$scope.showGetStarted = hashSizeFilter($scope.projects) === 0;

// if projectToDelete is in URL and if project exists to be deleted
if ($location.search()['projectToDelete']) {
Copy link
Member

Choose a reason for hiding this comment

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

instead of passing this as a URL parameter, I'd prefer this was just done in memory, could have a flash messages service or something

@gruiz17
Copy link
Contributor Author

gruiz17 commented Sep 1, 2015

@sg00dwin @jwforres i'm actually going to make a few more changes

  • putting the delete modal in a separate file, rather than right in the HTML
  • using the delete modal for the project list view

@jwforres
Copy link
Member

jwforres commented Sep 1, 2015

@gruiz17 our QE team opened a bug for the error message that appears when a viewer tries to delete a project and doesn't have permissions, currently it dumps a raw API blob, should probably be pulling the error message out when we have one, see https://bugzilla.redhat.com/show_bug.cgi?id=1258720

@gruiz17 gruiz17 force-pushed the master branch 2 times, most recently from 25d4416 to 9d07576 Compare September 1, 2015 20:21
@gruiz17
Copy link
Contributor Author

gruiz17 commented Sep 1, 2015

@jwforres I have fixed this! I show the error message now instead of the entire error api blob.

@sg00dwin I've also refactored the projects page to use the modal instead.

@gruiz17
Copy link
Contributor Author

gruiz17 commented Sep 1, 2015

@jwforres in my latest commit, i've also switched to using a service instead of url params when redirecting from settings to home 😄

.service("AlertMessageService", function(){
var alerts = [];
return {
addAlert: function(alert) {
Copy link
Member

Choose a reason for hiding this comment

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

need a way to clear the alerts or once you have added an alert to this its going to show up every time you navigate to the project list

@jwforres
Copy link
Member

jwforres commented Sep 2, 2015

attach screenshots to the PR for what it looks like now

@gruiz17 gruiz17 force-pushed the master branch 2 times, most recently from 80a5b35 to 4c3d023 Compare September 2, 2015 17:53
@gruiz17
Copy link
Contributor Author

gruiz17 commented Sep 2, 2015

@jwforres i've made all the fixes based on your feedback.

here is a gif!

project-settings-pr

@gruiz17
Copy link
Contributor Author

gruiz17 commented Sep 2, 2015

modal

image

flow from projects page

project-settings-pr-4

remove delete project modal html

several changes
* don't show alert for just any project in the query string
* UI fixes on modal
* rename functions for clarity

go bin

Additions to delete project functionality. CSS and markup

- custom delete-modal style
- use trashcan icon instead of delete button to be consistent with delete action from project list view
- minor positioning tweaks and removal of inline styles where not needed.

TODO:
- use delete modal when deleting from project list view

changes to projects and settings:
* add modal controller
* put modal in separate html file
* refactor projects page to use modal
* remove delete/cancel buttons from projects page
* make error messages friendly on UI instead of showing the entire API output

remove mistakenly added files

add alert service instead of using url params

remove unnecessary comment

minor changes:
* redo comments
* remove copied over projectDelete stuff
* add clearing alerts to alertmessageservice
* abstract loadprojects

scope fix for showing project name

scope fix on projects to make project name show up in project view
@jwforres jwforres added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2015
@jwforres
Copy link
Member

jwforres commented Sep 2, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/4738/) (Image: devenv-fedora_2275)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 3a55535

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/4738/)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 3a55535

openshift-bot pushed a commit that referenced this pull request Sep 3, 2015
Merged by openshift-bot
@openshift-bot openshift-bot merged commit c4a7c55 into openshift:master Sep 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants