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

Tweak settings level and add changed-only toggle #2882

Merged
merged 14 commits into from
Mar 17, 2024

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Nov 29, 2023

What does this implement/fix?

Remove settings level selection in favor of a simple settings level toogle in the same place:

Screenshot from 2023-11-29 13-13-53
Screenshot from 2023-11-29 13-13-55

Furthermore, we add a feature requested on Discourse: A toggle in the empty place on the All settings page that allows you to hide all default-valued config options:

Screenshot from 2023-11-29 13-14-21
Screenshot from 2023-11-29 13-14-24


Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


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.
  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)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

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

DL6ER commented Nov 29, 2023

Changed the color in the last commit to match the icon color:

grafik

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.

Screenshot at 2023-11-30 21-25-13

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER requested a review from yubiuser November 30, 2023 21:06
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.

Peek 2023-11-30 22-14

Switching to "All settings" sets the level to basic and no items are shown.

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.

Suggesting

settings.js L44 to change title="This is an advanced level setting to title="This is an expert level setting

@rdwebdesign
Copy link
Member

title="This is an expert level setting"

Maybe we should remove the "This is" part and use only title="Expert level setting".

style/pi-hole.css Show resolved Hide resolved
settings-all.lp Show resolved Hide resolved
…is page

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…igs/all

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

DL6ER commented Dec 1, 2023

When users decide to manually browse to https://pi.hole/admin/settings/all they will see:

Screenshot from 2023-12-01 10-02-09

@DL6ER DL6ER force-pushed the tweak/settings_changed_only branch from e34c51c to 132018b Compare December 1, 2023 12:02
@rdwebdesign
Copy link
Member

Sending users to a different page and then back doesn't seem like the best solution. It's a bit of a bad user experience.

Suggestions - If this page is accessed using basic level:

  1. show the level toggle or
  2. do not allow and redirect to /settings/system or /index
  3. automatically set Expert level (this is not an access level intended to block users without permissions, it is just a way to simplify the settings pages (view level). The user can already change the level whenever they wish).

@DL6ER
Copy link
Member Author

DL6ER commented Dec 1, 2023

You have to consider that nobody may ever see this. It's the edge case of the edge case

To me no. 2 sounds best, least obscure behavior in contrast to magically changing boxes or silent settings changes

…gs level is Basic

Signed-off-by: DL6ER <dl6er@dl6er.de>
rdwebdesign
rdwebdesign previously approved these changes Dec 2, 2023
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.

Some items lost/miss the "expert level" icon:

DNS/Conditional forwarding
Web/Exclusions
Web/Active sessions
Privacy/Privacy level

What shall we do with boxes that are no "settings" (don't allow interactions) but are also not shown in "basic" (eg. some boxes in system) - shall they get an icon?

@DL6ER
Copy link
Member Author

DL6ER commented Dec 2, 2023

So far, the settings levels (now reduced to two) were set in the web interface along - independently of FTL's flag ADVANCES_SETTING. This should probably be brought together somehow. I'm not sure how to do this best without the boxes first appearing and then disappearing again (or the other way around) as the AJAX API call will in every case have some (even it small) delay. Ideas?

@yubiuser
Copy link
Member

yubiuser commented Jan 7, 2024

I think this PR is a nice addition by reducing the overall number of "levels" and adding the "show only changed" items. This is worth to be implemented. In a subsequent step, we can think about how to deal with the "doubled" FTL and web flags and how/if we could combine them.

@yubiuser yubiuser closed this Jan 7, 2024
@yubiuser yubiuser reopened this Jan 7, 2024
@yubiuser
Copy link
Member

yubiuser commented Jan 8, 2024

We should use the same symbols for "expert level" boxes on the the settings pages (currently wrench) and in "all settings" (cog)

@yubiuser
Copy link
Member

yubiuser commented Jan 8, 2024

  1. We still have a few mixture of "advanced" and "expert" across various files. This should be unified.
  2. We decided to call it "settings-all" > this should be reflected also by renaming "settings-advanced.js" to "settings-all.js"

@yubiuser
Copy link
Member

yubiuser commented Jan 8, 2024

Thinking about

In a subsequent step, we can think about how to deal with the "doubled" FTL and web flags and how/if we could combine them.

Why do we need this FTL flag at all? We currently use it in two places: on various setting pages to determine if at least one "setting" within a "box" has the flag and set the wrench symbol. And on "all settings" to set the cog symbol for each setting box.

My proposal: remove the flag entirely. On the setting page, manually set the wrench to the box if we think the box contains something considered "advanced/expert". On "all settings" this symbol is useless > "all settings" is only reachable if "Expert level" has been selected already. If a user is so advanced to play around in "all settings" there is no need to highlight some settings as "advanced"

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@DL6ER
Copy link
Member Author

DL6ER commented Feb 11, 2024

The last comment makes this PR depend on pi-hole/FTL#1883

Copy link
Contributor

Conflicts have been resolved.

@DL6ER DL6ER requested a review from a team February 11, 2024 09:18
@DL6ER DL6ER removed the stale label Feb 11, 2024
@PromoFaux
Copy link
Member

Tested (both PRs) and working well. A couple of comments from my perspective (by no means mandatory for approval and up for discussion/refusal!)

On the All Settings page, after toggling to show only modified settings, the group headers are still visible if there are no modified settings within that group. Could those also be removed?

On the Local DNS Settings page (which is only visible with the Expert mode toggled), if you toggle the switch back to basic, you are left on the Local DNS Settings page but with zero options. Could a redirect to System be implemented in this case?

…ly modified settings if there are no modified settings within that group

Signed-off-by: DL6ER <dl6er@dl6er.de>
… Expert to Basic settings, redirect to admin/settings/system instead

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

rdwebdesign commented Mar 15, 2024

When the page is loaded, the checkbox is visible before the toggle buttons are created:

checkbox

I'm adding a commit to hide it using CSS.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@DL6ER DL6ER merged commit 7d51f40 into development-v6 Mar 17, 2024
8 checks passed
@DL6ER DL6ER deleted the tweak/settings_changed_only branch March 17, 2024 19:35
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.

None yet

4 participants