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

Changed HTML output to number input for Brightness, Speed, Intensity and Custom Sliders #4254

Open
wants to merge 2 commits into
base: 0_15
Choose a base branch
from

Conversation

Svennte
Copy link
Contributor

@Svennte Svennte commented Nov 6, 2024

Enhancement: Convert HTML Output Elements to Number Inputs for Improved Precision

Sometimes, I struggled to input specific values using the range sliders for effects and brightness. To improve precision, I changed the HTML output elements for Brightness, Speed, Intensity, and Custom Sliders to number inputs. I kept changes minimal to fit within the current code structure.

Tested with

  • Branch: 0_15
  • Device: esp32dev

What Have I Changed

index.html

  • Deleted HTML output elements and inserted HTML number inputs.
  • Added this.value as an argument for the Effects/Brightness setter functions.
  • Added this.checkValidity() to ensure only valid values are accepted for number inputs.

index.css

  • Renamed .sliderbubble class to .sliderInput and adjusted styles accordingly.
  • Slightly reduced .sliderwrap class width to display the number input properly.
  • Added styles to visually indicate invalid values in slider number inputs

index.js

  • Added val as an argument for the Effects/Brightness setter functions, allowing both range and number inputs to function.
  • Changed updateTrail to update the input instead of the output.
  • Removed the now-unnecessary slider bubble event.
  • Removed the toggleBubble function.

Issues

  • The inputs function correctly across tested devices and browsers. However, in the WLED Android App, the on-screen keyboard doesn’t push the slider div to the top of the display (see screenshots below). This issue only affects the WLED Android App and does not occur in the WLED Native App or web clients.
  • Just tested on ESP32dev, no other environments
  • Not tested with apple devices/browser

Questions

I couldn't find functions that redefine min/max values for the range inputs. Is this correct, or is there something specific to handle min/max values that I may have overlooked?

Android
Firefox
Android_Firefox
WLED-Native
Android_WLED-Native
WLED-Standard
Android_WLED-Standard
Windows
Chrome
Windows_Chrome
Firefox
Windows_Firefox
Invalid value
Windows_InvalidValue

@Svennte Svennte changed the title Convert Brightness, Speed, Intensity, and Custom Sliders from HTML output to number input Changed HTML output to number input for Brightness, Speed, Intensity and Custom Sliders Nov 6, 2024
@softhack007
Copy link
Collaborator

Thanks for your proposal!

personally I'm not an expert on html and JS, but I can answer one question

I couldn't find functions that redefine min/max values for the range inputs.

Ranges for all sliders (global brightness, effect sliders) are generally 0..255. The only exception is the "custom3" slider which has a reduced resolution of 0..31.

@Svennte
Copy link
Contributor Author

Svennte commented Nov 7, 2024

Ranges for all sliders (global brightness, effect sliders) are generally 0..255. The only exception is the "custom3" slider which has a reduced resolution of 0..31.

Okay, then the inputs are fine.
Is there something I have to do now?

@blazoncek
Copy link
Collaborator

blazoncek commented Nov 11, 2024

Will fix #3019 but I would recommend restoring tabs which were replaced by spaces in JS file.

EDIT: I'd also try to reuse range from existing (hidden) input fields instead of introducing new ones.

@Svennte
Copy link
Contributor Author

Svennte commented Nov 11, 2024

Thanks for your recommendations!

> I would recommend restoring tabs which were replaced by spaces in JS file.
Tabs have been restored with the last commit.

> EDIT: I'd also try to reuse range from existing (hidden) input fields instead of introducing new ones.
While this is an interesting idea, I have some concerns about implementation. Here’s my reasoning:

  • Maintaining Readability and Manageability: Reusing input ranges would require detaching and reattaching inputs on each tab change, such as when switching between segment value inputs.
  • Complexity of Custom Logic: This approach would demand custom logic to dynamically set min/max values, and bind onchange, oninput, and possibly other functions.
  • Potential Future Issues: To ensure that future UI changes wouldn’t introduce unexpected issues, we’d likely need an input manager. This manager would handle adding, removing, and modifying inputs based on a config file for each tab.
  • Risk of Misuse Across Tabs: Without the input manager, it might be challenging to track which input is in use on each tab, especially as this approach scales to more inputs.

If you have a simpler solution in mind that I may have overlooked, please let me know. Given the required work and the limited benefits, this approach seems impractical, especially since the inputs don’t share the same parent element.

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