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

Adds the ability to delete multiple warnings at once #2133

Merged
merged 9 commits into from
Mar 2, 2022

Conversation

rdwebdesign
Copy link
Member

@rdwebdesign rdwebdesign commented Feb 25, 2022

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

This PR adds new controls to the messages table to allow selection and deletion of multiple warnings at once.

How does this PR accomplish the above?:

This PR adds Datatables extensions, buttons and CSS.
Fix: update icon count after delete a warning. Also remove the triangle icon after delete last warning.

What documentation changes (if any) are needed to support this PR?:
none.

@rdwebdesign rdwebdesign force-pushed the deleteAll_warnings branch 2 times, most recently from de1ee1a to 6db37c1 Compare February 25, 2022 23:24
@rdwebdesign rdwebdesign requested a review from a team February 26, 2022 00:14
@DL6ER
Copy link
Member

DL6ER commented Feb 26, 2022

Something isn't right about the messages when mass-deleting:

Screenshot from 2022-02-26 10-13-47

The blue boxes never turn to green. The deletion still worked for all selected messages. It would be cleaner if there is only one popup saying that message(s) # 6491,6492,6493,6494,6495 have been deleted instead of five individual boxes.

PS: Purely cosmetic but maybe we should disable the buttons when there are no issues in the table:

Screenshot from 2022-02-26 10-12-02

Also I'm wondering about the position of these buttons. Mostly thinking about mobile displays that will not show the entire page at once: should they be at the bottom of the table? You typically scroll down on a page when doing the selections. Or we might want to have them both at the top and the bottom.

@DL6ER
Copy link
Member

DL6ER commented Feb 26, 2022

More thoughts about usability:

  1. Replace the "inversion" by a "select all" button. I don't think there is really any need for inversion and it takes one more step to realize that an inversion of an empty selection is effectively the same as
  2. If we have them both at the top and the bottom, it might be cleaner to use icons instead of text: and:
    1. Use the trash icon for delete (proposal: show the icon only if at least one item is selected),
    2. Use another icon for the proposed Select All button. Here are some icons I consider useful in this context: check, check-square, bars, ballot, check-double

@rdwebdesign rdwebdesign force-pushed the deleteAll_warnings branch 5 times, most recently from 7b5d65f to fb44f22 Compare February 26, 2022 20:35
- add Datatables extension Select (CSS and js);
- add Datatables extension Buttons (CSS and js);
- allow selection of multiple warnings;
- new button to select all;
- new button to delete all selected warnings;
- add icons for the buttons;
- hide buttons if all messages were deleted;
- update CSS themes to format the new items;
- Fix: update icon count after warning deletion;
- Fix: remove triangle icon after delete last warning;

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign
Copy link
Member Author

  • The texts were replaced by icons, on the buttons.
    image
  • I included buttons at the end of the table, too.
  • Now, the buttons only show up if there are messages on the list.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

  1. Form a UX perspective, I would expect a "deselect all" happening once all items have been selected an I click on the "select all" button again
  2. Although I have enough space, the new column adds a horizontal scrolling bar which does not look very nice.

Bildschirmfoto zu 2022-02-28 09-40-07

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign
Copy link
Member Author

  • buttons appear only when needed:
    • "Select All" when nothing is selected;
    • "Delete Selected" only if something is selected;
    • "Select All" (with different icon) when one or more items are selected;
    • "Deselect All" when whole page is selected;
  • show buttons at the top and bottom of the table;
  • batch deletion accepts array (multiple notifications fixed);
  • fixed horizontal bar for long warning messages.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
DL6ER
DL6ER previously approved these changes Mar 1, 2022
scripts/pi-hole/js/messages.js Outdated Show resolved Hide resolved
scripts/pi-hole/js/messages.js Outdated Show resolved Hide resolved
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign requested a review from yubiuser March 1, 2022 18:33
@yubiuser yubiuser merged commit a329c4f into devel Mar 2, 2022
@yubiuser yubiuser deleted the deleteAll_warnings branch March 2, 2022 05:46
@yubiuser
Copy link
Member

yubiuser commented Mar 2, 2022

Feature request: port this to all other tables 😁

@rdwebdesign
Copy link
Member Author

Feature request: port this to all other tables 😁

We receive a high number of requests, but as a free open-source project run by volunteers we have very limited resources.
Please, we ask that you open your Feature Requests at our Discourse Forum.

Thank you for your understanding.

🤣 🤣 🤣

@yubiuser
Copy link
Member

yubiuser commented Mar 2, 2022

@rdwebdesign rdwebdesign mentioned this pull request Mar 24, 2022
9 tasks
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-15-web-v5-12-and-core-v5-10-released/54987/1

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/one-click-delete-mark-as-read-all-warnings/53097/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popups overlay action items when action items are clicked
4 participants