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

Add the ability to filter domains by type #2334

Merged
merged 4 commits into from
Sep 10, 2022
Merged

Add the ability to filter domains by type #2334

merged 4 commits into from
Sep 10, 2022

Conversation

rdwebdesign
Copy link
Member

@rdwebdesign rdwebdesign commented Sep 5, 2022

What does this PR aim to accomplish?:

After a recent update, all domains are shown on the same page.

This PR adds the ability to filter domains by type, allowing turn on/off the visibility of:

  • Exact whitelists;
  • Exact blacklists;
  • Regex whitelists;
  • Regex blacklists.

How does this PR accomplish the above?:

Adding a filter and checkboxes to control the filter.

image

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

none.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

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.

I deselected all checkboxes, but instead of showing no domains it switched back to show all domains. This is kind of counter-intuitive. Is this the expected behavior?

@DL6ER
Copy link
Member

DL6ER commented Sep 6, 2022

I wonder if this makes the page again somewhat more complex to use. This also requires extra amounts of space, if we keep it, could we explore having this in the box-header instead of in a dedicated row?

@rdwebdesign
Copy link
Member Author

Answering each question:

Is this the expected behavior?

Yes.
Actually, the initial behavior was to have all checkboxes deselected, showing everything. If you select something, everything would disappear except the selected one. If you select another, it would be added to the selection.
After testing, I decided to start with all selected, but keep the "show all", if none are selected (I don't see a reason to hide all).

This also requires extra amounts of space, if we keep it, could we explore having this in the box-header instead of in a dedicated row?

I thought about that and I think is a great idea. I only put the filter with the other controls because I think nobody would like it there.

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.

I like the new position of the checkboxes, it fits in there very well. I would just change the order of "Exact blacklist" and "RegEx whitelist", so both withlist and both blacklist boxes are grouped. This makes it easier to "restore" the old function to only see whitelist/blacklist entries.


Is this the expected behavior?

Yes.
After testing, I decided to start with all selected, but keep the "show all", if none are selected (I don't see a reason to hide all).

Sure it makes no sense to deselect all and expect any output - however, I would argue one would expect to see an empty table then.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
- change checkboxes order;
- remove all lines if all checkboxes are unselected.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign merged commit 2c1db2c into devel Sep 10, 2022
@rdwebdesign rdwebdesign deleted the filter_domains branch September 10, 2022 00:19
@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/color-coding-domain-type/57763/3

@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/i-cant-find-my-whitelist-in-the-new-version/57759/5

@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/whitelist-and-blacklist/57563/9

@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-18-web-v5-15-and-core-v5-12-1-released/57894/1

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.

4 participants