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

New layout for "All Settings" page #3042

Merged
merged 17 commits into from
Jun 6, 2024
Merged

Conversation

rdwebdesign
Copy link
Member

What does this PR accomplishes?

This PR creates a cleaner layout for All Settings page, removing a few boxes and borders.

It also improves the number of columns, depending on the screen size (1 column in small and medium screens, 2 columns in large screens and 3 columns in very large screens).


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)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

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

@rdwebdesign rdwebdesign force-pushed the new/settings_all_layout branch 3 times, most recently from db33e79 to 7c823de Compare June 4, 2024 20:10
also surround the icons with a SPAN tag.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
- add class `row` to form-group (because it contains columns) and move
  the tag to the details function
- add class `col-sm-12` to boolean values
- add new class `settings-box` to allow targeting only settings boxes

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 label style and width
- set icons padding

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
This replaces the original `<select>` and merges the options with the
"allowed values" block in a single one, simplifying the presentation.
Also, the defaultValueHint was incorporated into the default value.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
- Adjust styles to show only one column in mid and small screens
- also group all "settings" styles

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
- remove the external green boxes;
- remove border from inner boxes (and change the box-shadow);
- add navigation and tabs for settings sections;
- adjust/add some classes for formatting

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
and fix the navigation basic style colors

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign force-pushed the new/settings_all_layout branch from 7c823de to 5db10ae Compare June 4, 2024 21:02
@rdwebdesign rdwebdesign marked this pull request as ready for review June 4, 2024 21:03
@rdwebdesign rdwebdesign requested a review from a team June 5, 2024 01:15
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 really like this change. It looks much nicer with the tabs. But as a user I wouldn't know if I need to save after changing settings in one tab or if I could do changes on multiple tabs and only save once. Any idea how to let the user know?

scripts/pi-hole/js/settings-advanced.js Show resolved Hide resolved
@PromoFaux
Copy link
Member

Overall, I like this change - it looks really smart. One possible change request, however... Could we make it more obvious that the tab headers are, in fact, tab headers? Took me a moment to notice that they were there as the background colour of the buttons is the same as the page background - doesn't visually stick out as a tabbed layout (to my eye!)

@yubiuser
Copy link
Member

yubiuser commented Jun 5, 2024

One small drawback with tabs is that you can't search across all settings right away eg. directly jump to a certain debug setting.

@PromoFaux
Copy link
Member

Ah yes, I suppose currently you can load the page and ctrl+f

I wonder, as the data is technically still on the page (just hidden with some CSS?) Is there some super magic js/css that can be employed here to make the tab get selected and shown based on the contents of the ctrl+f search string? Or am I wishing for the impossible? I suppose a simple search box on at the top could be easier to wire up...

@rdwebdesign
Copy link
Member Author

rdwebdesign commented Jun 5, 2024

Well... with tabs there will be less visible content on each tab, making it easier to visually search, but slightly more difficult to search using CTRL + F (unless you know which tab to look for).

Or am I wishing for the impossible?

Not possible, because display: none really hides the content.

@PromoFaux
Copy link
Member

Sorry - having "ideas" again...

When toggling to modified settings only, could we either:
a) Add some sort of indication to a tab that contains modified settings
b) Remove any tabs that have no modified settings to display

Might save a few clicks when looking for modified settings

Not possible, because display: none really hides the content.

I figured that would be the case. What about a custom search box at the top of the all settings page, above the tabs/next to the toggle switch

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>
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign force-pushed the new/settings_all_layout branch from 6303c2d to 20fc994 Compare June 5, 2024 23:32
@rdwebdesign rdwebdesign requested review from PromoFaux and a team June 6, 2024 00:16
@DL6ER
Copy link
Member

DL6ER commented Jun 6, 2024

I do like this a lot. It even keeps adaptivity so when FTL decides at any point to add a new main category, we do not need to change the web code to make it appear:

image

Note: This is a real example when running FTL branch new/ntp

@rdwebdesign rdwebdesign merged commit 5b6c8bb into development-v6 Jun 6, 2024
8 checks passed
@rdwebdesign rdwebdesign deleted the new/settings_all_layout branch June 6, 2024 22:13
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.

4 participants