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

Use the setupVars.conf option TEMPERATUREUNIT, plus slight rearrangement of settings page #2516

Merged
merged 4 commits into from Feb 9, 2023

Conversation

rdwebdesign
Copy link
Member

@rdwebdesign rdwebdesign commented Jan 28, 2023

What does this PR aim to accomplish?

Allow the selection of temperature unit shown on the web interface via setupVars.conf, like other interfaces (PADD/chronometer) already do.

Currently the web interface always defaults to "C" (Celcius). The user must manually select the unit on Setting page for every device/browser used.

Note:
The suggest code here will use a global unit and remove the ability to use different units on each device (which was probably never the desired behavior).

How does this PR accomplish the above?

Changing the code to use a new behavior:

  • if the option TEMPERATUREUNIT is set in setupVars.conf file, the value will be used.
  • if there is no unit set in setupVars.conf, but the browser has an old value set on locastorage, the localstorage value will be used and stored in setupVars.conf file.
  • if there is no unit set in setupVars.conf or localstorage, "C" will be used as default value.

Also:

  • The temperature unit will be set GLOBALLY and not per browser.
  • changing the value on the web interface WILL CHANGE setupVars.conf.

Link documentation PRs if any are needed to support this PR:

The TEMPERATUREUNIT description on docker-pi-hole repository will need to be updated to reflect the new behavior


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

New behavior!!!
The temperature unit will be set GLOBALLY and not per browser.

- if a TEMPERATUREUNIT is set in setupVars.conf file, the value will be used.
- if there is no unit set in setupVars.conf, "C" will be used;
- changing the value on the web interface WILL CHANGE setupVars.conf.
- if the browser has an old value set on locastorage, this will be ingnored.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign rdwebdesign requested review from DL6ER and a team January 28, 2023 20:02
Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@PromoFaux
Copy link
Member

App appears to work, though regarding this part:

The temperature unit will be set GLOBALLY and not per browser.

With that in mind, we should probably move this out of the Per-browser settings box, and into the Web interface settings box?

@rdwebdesign
Copy link
Member Author

With that in mind, we should probably move this out of the Per-browser settings box, and into the Web interface settings box?

I agree.

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

I think it looks fine, however it now introduces something else... Changing the temperature unit is automatically updated when switching between them without having to press save...

The other options in that same dialog require the usage of the save button. Two suggestions:

  • Make the others change on selection rather than pressing save
  • Separate the temperature unit selection out to another box / separate part of the same box to show that Save is not needed for this item

@rdwebdesign
Copy link
Member Author

Yeah... I notice this behavior.

What do you think about this?
web_api

@PromoFaux
Copy link
Member

Yep - that looks good to me

- use separate tabs for Web and API
- change box titles and options distribution

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@PromoFaux PromoFaux changed the title Use the setupVars.conf option TEMPERATUREUNIT Use the setupVars.conf option TEMPERATUREUNIT, plus slight rearrangement of settings page Feb 2, 2023
@PromoFaux PromoFaux merged commit acb7892 into devel Feb 9, 2023
@PromoFaux PromoFaux deleted the temp_unit_no_localstorage branch February 9, 2023 18:36
@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-21-web-v5-18-4-and-core-v5-15-4-released/61096/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.

None yet

3 participants