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

Improve filtering on table pages #1222

Merged
merged 30 commits into from
Jun 23, 2020
Merged

Improve filtering on table pages #1222

merged 30 commits into from
Jun 23, 2020

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Apr 22, 2020

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, and have included unit tests where possible.
  • 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)

What does this PR aim to accomplish?:

Improve filtering on table pages. This enables a whole set of new features, see https://discourse.pi-hole.net/t/explicit-search-in-pi-hole-admin-console/28893/16

How does this PR accomplish the above?:

Change on-click actions on query types, domain and client from searching to filtering

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

None

@DL6ER DL6ER force-pushed the tweak/exact_searching branch from ec1434b to d16ed04 Compare April 24, 2020 10:06
@DL6ER DL6ER force-pushed the tweak/exact_searching branch 2 times, most recently from 136f713 to e46269b Compare April 25, 2020 04:34
@DL6ER DL6ER force-pushed the tweak/exact_searching branch from e46269b to 82f4549 Compare April 25, 2020 04:39
@DL6ER DL6ER force-pushed the tweak/exact_searching branch 2 times, most recently from 46b7aba to efe0244 Compare April 26, 2020 07:57
@DL6ER DL6ER force-pushed the tweak/exact_searching branch from efe0244 to cb3be64 Compare April 26, 2020 07:57
DL6ER added 2 commits April 26, 2020 21:51
…ot lost when removing a column from the selection.

Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
@DL6ER
Copy link
Member Author

DL6ER commented Jun 4, 2020

I addressed the raised review points:

  • The button text is not short and static. Filtering is obvious enough from the highlighted columns
  • I fixed the highlight color. They got lost in the adjustments for the light/dark themes. The highlighting colors of both themes are now set up correctly (it is intentional to change them theme-wide).
  • I added both click pointers as well as dynamic tooltips:
    Screenshot from 2020-06-04 10-06-18
    Screenshot from 2020-06-04 10-11-56
    (the cursor isn't shown in the screenshots, however, it is of pointer type.

@yubiuser
Copy link
Member

yubiuser commented Jun 4, 2020

Thanks I like it a lot.

@yubiuser
Copy link
Member

yubiuser commented Jun 4, 2020

Initially filtering on status and reply was not possible due to technical reasons. Later you added the ability.
Does this now also enable the ability to search these columns? If not possible maybe add "Type/Domain/Client" as light grey text in the search input field so users know that it is limited to those columns.

@DL6ER
Copy link
Member Author

DL6ER commented Jun 4, 2020

Initially filtering on status and reply was not possible due to technical reasons. Later you added the ability.

Where did I add this? 🤔
I don't think it is needed, there is a rather limited set of possibilities for these fields and filtering-on-click should be totally sufficient. Still, the placeholder is a good idea.

Screenshot from 2020-06-04 10-52-40

@DL6ER
Copy link
Member Author

DL6ER commented Jun 4, 2020

Ah, I see. This was a misunderstanding. Yes, filtering on click is possible, but not through the search. The new placeholder should clarify this. Thanks.

PromoFaux
PromoFaux previously approved these changes Jun 4, 2020
@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/sort-query-list-show-specific-client-dropdown-all-piholed-ok/15039/3

@DL6ER
Copy link
Member Author

DL6ER commented Jun 14, 2020

@PromoFaux Kindly requesting re-approval.

style/themes/default-dark.css Outdated Show resolved Hide resolved
@DL6ER DL6ER force-pushed the tweak/exact_searching branch from 429c21a to f06a2b6 Compare June 21, 2020 19:45
Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

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

Functionally speaking, this works well.

@DL6ER DL6ER dismissed XhmikosR’s stale review June 23, 2020 19:26

Requested changes were made

@DL6ER DL6ER merged commit 59bd4de into devel Jun 23, 2020
@DL6ER DL6ER deleted the tweak/exact_searching branch June 23, 2020 19:26
@PromoFaux PromoFaux mentioned this pull request Jul 5, 2020
@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-5-1-released/35577/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/extend-query-table-filtering-from-v5-1-to-long-term-data-query-log/35634/1

@reinstej-sci
Copy link

Were consequences of this change on mobile/tablet browsers considered? This has broken the ability to click to filter on mobile.

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

Successfully merging this pull request may close these issues.

6 participants