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.

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

[configuration] adds confirmation prompt when deleting a field. #7069

Merged
merged 7 commits into from
Sep 24, 2021
Merged

[configuration] adds confirmation prompt when deleting a field. #7069

merged 7 commits into from
Sep 24, 2021

Conversation

racostas
Copy link
Contributor

Brief summary of changes

Adds confirmation prompt when deleting a field.
See more details on issue #7010

  • Have you updated related documentation?
    Test Plan updated.

Testing instructions

  1. Go to MainMenu-> Admin -> Configuration -> Statistics.
  2. Click on the "X" button for an already submitted(existing) field.
  3. A dialog box should ask for delete confirmation before proceeding.

Link to related issue

@racostas racostas added Category: Bug PR or issue that aims to report or fix a bug Area: UI PR or issue related to the user interface labels Sep 28, 2020
@cmadjar
Copy link
Collaborator

cmadjar commented Jun 8, 2021

@racostas could you please rebase so the tests can run?

@zaliqarosli could you review this PR once rebased please? Thank you!

@zaliqarosli zaliqarosli reopened this Jun 21, 2021
zaliqarosli
zaliqarosli requested changes Jun 22, 2021
View reviewed changes
modules/configuration/test/TestPlan.md Outdated Show resolved Hide resolved
modules/configuration/test/TestPlan.md Outdated Show resolved Hide resolved
modules/configuration/js/configuration_helper.js Outdated Show resolved Hide resolved
@cmadjar
Copy link
Collaborator

cmadjar commented Aug 10, 2021

@zaliqarosli could you re-review please? Thanks!

@laemtl laemtl self-assigned this Aug 10, 2021
@zaliqarosli
Copy link
Contributor

status update: @laemtl may have a solution for us (to use sweetalert2 on non-react module). i will re-review after this has been implemented

@zaliqarosli zaliqarosli added the State: Needs work PR awaiting additional work by the author to proceed label Aug 12, 2021
@laemtl
Copy link
Contributor

laemtl commented Aug 14, 2021

@zaliqarosli Fix is ready and waiting for permission to push.

@racostas
Copy link
Contributor Author

Go ahead @laemtl ! Thank you.

@laemtl
Copy link
Contributor

laemtl commented Aug 16, 2021 •
edited
Loading

@racostas pushed.

@laemtl laemtl force-pushed the configurationFieldDeletionConfirmationDialog branch from 90cd422 to b8ccee1 Compare August 16, 2021 15:38
@laemtl
Copy link
Contributor

laemtl commented Aug 16, 2021

@racostas I fixed a conflict with a rebase.

@laemtl laemtl removed the State: Needs work PR awaiting additional work by the author to proceed label Aug 16, 2021
@laemtl laemtl force-pushed the configurationFieldDeletionConfirmationDialog branch from 3c810c6 to 28be440 Compare August 16, 2021 17:32
@zaliqarosli
Copy link
Contributor

@laemtl thanks for the contribution! is this ready to test? I'm wondering if modules/configuration/js/configuration_helper.js can be completely removed given the new jsx file?

@laemtl
Copy link
Contributor

laemtl commented Aug 18, 2021

@zaliqarosli Yes, ready to test! And thanks for letting me know about the duplicates, it was a mistake.

zaliqarosli
zaliqarosli requested changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

add js/configuration_helper.js to the .gitignore file, but besides that, everythings great!

@zaliqarosli zaliqarosli added the Passed manual tests PR has been successfully tested by at least one peer label Aug 18, 2021
@laemtl
Copy link
Contributor

laemtl commented Aug 19, 2021

@zaliqarosli .gitignore updated!

zaliqarosli
zaliqarosli approved these changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

LGTM

@driusan
Copy link
Collaborator

driusan commented Aug 26, 2021

Seems to be failing tests?

@laemtl
Copy link
Contributor

laemtl commented Aug 30, 2021

@driusan No tests have run successfully for a couple of days now. They need to be fixed, unrelated to this PR.

@laemtl laemtl force-pushed the configurationFieldDeletionConfirmationDialog branch 3 times, most recently from 2a7d8f7 to 503c5d9 Compare August 30, 2021 16:16
@laemtl laemtl force-pushed the configurationFieldDeletionConfirmationDialog branch from 503c5d9 to ea18c52 Compare September 2, 2021 18:44
@driusan driusan merged commit 8de5079 into aces:main Sep 24, 2021
@ridz1208 ridz1208 added this to the 24.0.0 milestone Nov 4, 2021
@ridz1208 ridz1208 mentioned this pull request Feb 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laemtl laemtl laemtl left review comments

@zaliqarosli zaliqarosli zaliqarosli approved these changes

Assignees

@zaliqarosli zaliqarosli

@laemtl laemtl

@racostas racostas

Labels
Area: UI PR or issue related to the user interface Category: Bug PR or issue that aims to report or fix a bug Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Milestone
24.0.0
6 participants
@racostas @cmadjar @zaliqarosli @laemtl @driusan @ridz1208