Skip to content

Implement application deleting on the web #1313

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
pipecd-bot merged 1 commit into from
Dec 22, 2020
Merged

Implement application deleting on the web #1313

pipecd-bot merged 1 commit into from
Dec 22, 2020

Conversation

@cakecatz
Copy link
Contributor

@cakecatz cakecatz commented Dec 22, 2020 •
edited
Loading

What this PR does / why we need it:

Kapture.2020-12-22.at.18.39.25.mp4

This screen capture uses a dummy API call, so the application row is not removed actually.

Which issue(s) this PR fixes:

Fixes #1290

Does this PR introduce a user-facing change?:

Add a delete option for application at the list page

@cakecatz cakecatz changed the title Implement application deleting on the web WIP: Implement application deleting on the web Dec 22, 2020
@pipecd-bot
Copy link
Collaborator

pipecd-bot commented Dec 22, 2020

COVERAGE

Code coverage for golang is 33.61%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

pipecd-bot commented Dec 22, 2020

COVERAGE

Code coverage for javascript is 84.97%. This pull request decreases coverage by -0.43%.

File Base Head Diff
src/modules/delete-application.ts -- 43.75% +43.75%
src/api/applications.ts 65.82% 64.63% -1.19%
src/constants/toast-text.ts 100.00% 100.00% +0.00%
src/constants/ui-text.ts 100.00% 100.00% +0.00%
src/modules/applications.ts 89.80% 90.00% +0.20%
src/modules/index.ts 100.00% 100.00% +0.00%

cakecatz
cakecatz commented Dec 22, 2020
View reviewed changes
pkg/app/web/src/components/delete-application-dialog.tsx
}));

const TITLE = "Delete Application";
const ALERT_TEXT = "Are you sure you want to delete the application?";
Copy link
Contributor Author

@cakecatz cakecatz Dec 22, 2020

Choose a reason for hiding this comment

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

If anyone has a better phrase, please let me know.

Copy link
Member

@nghialv nghialv Dec 22, 2020

Choose a reason for hiding this comment

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

Looks good to me.
"Are you sure you want to DELETE the application?"

Copy link
Member

@khanhtc1202 khanhtc1202 Dec 22, 2020

Choose a reason for hiding this comment

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

We could you the same text as github does 🤔
Screen Shot 2020-12-22 at 19 08 46

Copy link
Member

@nghialv nghialv Dec 22, 2020

Choose a reason for hiding this comment

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

👍

Copy link
Member

@nghialv nghialv Dec 22, 2020

Choose a reason for hiding this comment

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

The confirmation could be added/improved later.
Let's merge this pull request with the current design.

Copy link
Member

@khanhtc1202 khanhtc1202 Dec 22, 2020

Choose a reason for hiding this comment

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

🙆‍♀️ 🚀

@cakecatz cakecatz changed the title WIP: Implement application deleting on the web Implement application deleting on the web Dec 22, 2020
@nghialv
Copy link
Member

nghialv commented Dec 22, 2020

Thank you.
/lgtm

@khanhtc1202
Copy link
Member

khanhtc1202 commented Dec 22, 2020 •
edited
Loading

/approve

@pipecd-bot
Copy link
Collaborator

pipecd-bot commented Dec 22, 2020

APPROVE

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@pipecd-bot pipecd-bot merged commit fdf7a7b into master Dec 22, 2020
@pipecd-bot pipecd-bot deleted the delete-app branch December 22, 2020 10:20
@pipecd-bot pipecd-bot mentioned this pull request Dec 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khanhtc1202 khanhtc1202 Awaiting requested review from khanhtc1202

@nghialv nghialv Awaiting requested review from nghialv

Assignees

No one assigned

Labels

approved area/web lgtm size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add a delete option for application at the list page

5 participants

@cakecatz @pipecd-bot @nghialv @khanhtc1202