Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

feat(delete-work-item): add delete work item from list #2797

Merged
merged 4 commits into from
Nov 8, 2018
Merged

feat(delete-work-item): add delete work item from list #2797

merged 4 commits into from
Nov 8, 2018

Conversation

divyanshiGupta
Copy link
Contributor

@divyanshiGupta divyanshiGupta commented Oct 31, 2018 •
edited
Loading

Note: If there are pending changes to the PR, prefix the PR title with "WIP" and add the label "DO NOT MERGE"

Mandatory

  • What does this PR do?
    This PR adds the feature of delete work item from the query list,
    quick-preview and detail page.

  • Following depicts the flow for deleting a work item

  • From list
    screenshot from 2018-11-01 16-26-25
    screenshot from 2018-11-01 16-26-37

  • From preview
    screenshot from 2018-11-01 17-17-28
    screenshot from 2018-11-01 16-29-22
    screenshot from 2018-11-01 17-17-49

  • From detail page
    screenshot from 2018-11-01 17-18-03
    screenshot from 2018-11-01 16-29-59
    screenshot from 2018-11-01 16-30-12

  • What issue/task does this PR references?
    Story - https://openshift.io/openshiftio/Openshift_io/plan/detail/76

  • Are the tests Included?
    WIP

Optional

  • Is the documentation Included?

  • Are the Release Notes included?

  • @mention(s) to expected reviewer(s) included

@alien-ike
Copy link
Collaborator

alien-ike commented Oct 31, 2018 •
edited
Loading

Ike Plugins (test-keeper)

Thank you @divyanshiGupta for this contribution!

It seems that this PR already contains some added or changed tests. Good job!

Your plugin configuration is stored in the file.

@rohitkrai03
Copy link
Contributor

@divyanshiGupta Can you add some screenshots to the PR description? Also, add ux team for review since this is a UX change. cc: @fabric8-ui/uxd

@rohitkrai03
Copy link
Contributor

@divyanshiGupta Tests are failing for the delete work item workflow. Please take a look.

@divyanshiGupta
Copy link
Contributor Author

@rohitkrai03 I have made the changes you asked for and added few screenshots as well. I am working on fixing the tests.

sahil143
sahil143 suggested changes Nov 1, 2018
View reviewed changes
Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

Black color icon for delete is not looking good. Can we give it a color(maybe Red)?
@Veethika ^^

src/app/components_ngrx/delete-work-item/delete-work-item.module.ts Outdated Show resolved Hide resolved
src/app/effects/work-item.effects.ts Outdated Show resolved Hide resolved
src/app/effects/work-item.effects.ts Outdated Show resolved Hide resolved
@divyanshiGupta
Copy link
Contributor Author

@sahil143 actually I added this and some other suggestions as comments in the invision document. We can definitely change it to red.

@divyanshiGupta
Copy link
Contributor Author

@sahil143 I have made the changes and I have changed the icon color to red. cc: @Veethika

rohitkrai03
rohitkrai03 approved these changes Nov 2, 2018
View reviewed changes
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM code wise. Just a small quirk from my side. I can see workitem in some places and workItem in others. It should be consistent and camel cased workItem makes more sense. Not a deal breaker though since i am not sure how its spelled all over planner code.

@divyanshiGupta divyanshiGupta force-pushed the delete-workitem branch 2 times, most recently from a45257a to 227fd0a Compare November 2, 2018 11:06
@divyanshiGupta
Copy link
Contributor Author

@rohitkrai03 it makes sense and I have made the changes accordingly.

@rohitkrai03
Copy link
Contributor

@divyanshiGupta Great.

@divyanshiGupta
Copy link
Contributor Author

[test]

@divyanshiGupta divyanshiGupta force-pushed the delete-workitem branch 2 times, most recently from 87e629c to 8a7b789 Compare November 2, 2018 17:34
@centos-ci
Copy link
Collaborator

@divyanshiGupta Your image is available in the registry. Run docker pull quay.io/openshiftio/fabric8-ui-fabric8-planner:SNAPSHOT-PR-2797 && docker run -it -p 5000:8080 quay.io/openshiftio/fabric8-ui-fabric8-planner:SNAPSHOT-PR-2797 and visit http://localhost:5000 to access it.

@divyanshiGupta
Copy link
Contributor Author

[test]

@centos-ci
Copy link
Collaborator

@divyanshiGupta Your image is available in the registry. Run docker pull quay.io/openshiftio/fabric8-ui-fabric8-planner:SNAPSHOT-PR-2797 && docker run -it -p 5000:8080 quay.io/openshiftio/fabric8-ui-fabric8-planner:SNAPSHOT-PR-2797 and visit http://localhost:5000 to access it.

Raunak1203
Raunak1203 suggested changes Nov 7, 2018
View reviewed changes
Copy link
Contributor

@Raunak1203 Raunak1203 left a comment

Choose a reason for hiding this comment

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

@divyanshiGupta if the title is long, delete icon is moving out of the column
screenshot from 2018-11-07 13-39-05

Raunak1203
Raunak1203 suggested changes Nov 8, 2018
View reviewed changes
src/tests/specs/query-tab.spec.ts Outdated Show resolved Hide resolved
@divyanshiGupta
Copy link
Contributor Author

@Raunak1203 done.

@centos-ci
Copy link
Collaborator

@divyanshiGupta Your image is available in the registry. Run docker pull quay.io/openshiftio/fabric8-ui-fabric8-planner:SNAPSHOT-PR-2797 && docker run -it -p 5000:8080 quay.io/openshiftio/fabric8-ui-fabric8-planner:SNAPSHOT-PR-2797 and visit http://localhost:5000 to access it.

@divyanshiGupta
Copy link
Contributor Author

@nimishamukherjee can you merge this PR?

@nimishamukherjee
Copy link
Collaborator

Good work @divyanshiGupta

@nimishamukherjee nimishamukherjee merged commit c45d8bc into fabric8-ui:master Nov 8, 2018
@divyanshiGupta
Copy link
Contributor Author

Thank you @nimishamukherjee

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Raunak1203 Raunak1203 Raunak1203 approved these changes

@sudsen sudsen sudsen approved these changes

@sahil143 sahil143 sahil143 approved these changes

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@divyanshiGupta @alien-ike @rohitkrai03 @centos-ci @nimishamukherjee @sahil143 @sudsen @Raunak1203