Refactor two-way data binding using proxy by mohamedhyouns · Pull Request #38 · dxpr/DXB-Slider

mohamedhyouns · 2024-12-12T06:37:08Z

Fix Buggy behaviors in number input #32

- Refactored two-way binding logic to allow number inputs to remain empty. - Added `getRangePercent` function to compute slider percentage values. - Introduced `updateFieldValue` to handle empty or numeric inputs gracefully. - Implemented `sliderStateProxy` to synchronize range and number inputs. - Centralized input handling via `handleInputChange` to simplify event management. - Removed wired `addEventListener` logic to reduce bugs and improve maintainability.

- Added `name` attributes to slider inputs in `index.html` for proxy-based synchronization.

Removed test as the proxy now handles input and range updates automatically.

The current style of file not consistent with vscode reformating style

mohamedhyouns · 2024-12-12T07:09:49Z

After merging this and updating the DXBR builder with the new slider files, this bug will be valid and need to be fixed

mohamedhyouns · 2024-12-12T07:17:46Z

Resources:

Eirik Vullum: JavaScript Metaprogramming - ES6 Proxy Use and Abuse | JSConf Budapest 2017
Two-way data binding and reactivity with 15 lines of vanilla JavaScript

jjroelofs

I will also comment on the issue but the bug you found is not a bug but a feature, here is why:

The number input is not a real form element in this use case, it is just a tool that enhances the slider input with the possibility for more accuracy, that is why it also should not have a name attribute, we don't want to submit it separately to the server.
A range element's value cannot be cleared, "empty" is not a valid state for it. That is why allowing the number input to be empty would be a bug.
The default value of a range input is to be in the center. When clearing the number input, it slider goes to the center, and the number input assumes the value corresponding to the middle of range slider. This is expected behavior.

mohamedhyouns · 2024-12-16T07:31:09Z

@jjroelofs Please have a look https://dxpr.monday.com/boards/935035417/pulses/8055708426

mohamedhyouns · 2024-12-22T07:24:52Z

@jjroelofs, I got the idea of resetting both inputs to the middle when trying to clear the input of type number. This has been addressed here.

@jjroelofs Regarding the current logic that fires two events, I believe it exists as a workaround to avoid incorrect setups of listeners in the live editing manager. Since creating the input of type number in the DXB slider is an async process, this workaround seems to have been added to compensate. However, as a developer, I expect only one event listener to fire when interacting with either the input of type number or type range in the DXB slider. Debugging the code step by step reveals this behavior to be a bug.

@jjroelofs I see the issue I’m trying to solve in the current PR is still unclear. First, let me clarify that the current issue is an atomic issue that addresses a flaw in the architecture of how the DXB slider updates values for both inputs. IMHO, we need to adopt the new logic, and it’s not feasible to split this into smaller changes due to the nature of the issue.

Since this is an atomic issue addressing multiple bugs, do you suggest releasing it as 3.x?

I recorded a video to explain why the existing logic is problematic: https://www.youtube.com/watch?v=B8V_Bw9NK44.

jjroelofs

This PR is too complicated for me to review without having information about the engineering decisions that were made and why they were made, so I can't accept it without a good commit message and PR description.

Also, I don't like the requirement for a new data attribute. The attribute values added in the examples are identical to the id values with already have so it seems they are redundant. Please see if you can remove this requirement because this change makes implementation more error prone and it is not backwards compatible, meaning we would have to bump the version to 3.x.

mhmelshaaerdxpr

The new changes introduce new bugs:

Investment Amount slider example is broken.
The input validation is no longer working—i.e., you can set values outside the configured range or fraction of step value.

mhmelshaaerdxpr · 2025-01-20T08:36:16Z

dxb-slider.js

+          return;
+        }
+
+        sliderStateProxy[e.target.dataset["dxbProxyKey"]] = e.target.value;


Please be consistent, always use dataset.dxbProxyKey for getting and setting values. Don't mix setAttribute("data-attr-name", value) with dataset.attributeName.
Also, you may put the attribute name in a const variable and use it whenever needed instead of hardcoding the same attribute name in all uses.

5aa8e33
68ee689

mhmelshaaerdxpr · 2025-01-20T08:38:58Z

dxb-slider.js

+      [rangeInput, numberInput].forEach(input =>
+        input.addEventListener('input', handleInputChange)
+      );


Not sure if it's a good idea to have the same handler. For instance, we need to separate the numberInput handler and introduce a debounce to invoke the handler only after the user stops writing values—i.e., a 500ms debounce.

a4a1d4c
Adding debounce won't be a good idea, it will affect the UX I tested it with 500 and without denouncing

slider-debouce.mp4

mhmelshaaerdxpr · 2025-01-21T06:56:42Z

dxb-slider.js

+        // Reset the input value to the previously stored value if it exceeds the maximum allowed value
+        const newValue = Number(e.target.value);
+        const max = e.target.max;
+
+        if (max && newValue > e.target.max) {
+          e.target.value = sliderStateProxy[e.target.dataset["dxbProxyKey"]]
+        }
+


Please hand this over to the proxy not the event listener handler. This when Javascript Proxy are actually used for—i.e., validation.

mhmelshaaerdxpr

Please test your code thoroughly. This is the second instance where easily detectable bugs could have been caught with basic testing.

For ranges like [-100, 100] with default step, I cannot add negative values (it resets to mid range).
Value resets are confusing: Sometimes reset to max (i.e., when exceed maximum), reset to mid (i.e., on invalid input or on clear).
In the 100 step example, putting a fraction of a step value do approximation on the range field while keep the number as is in the input field. Did we decide at some point to do this approximation part? Regardless, there is a mismatch in behavior between the input and range fields.
We should provide feedback rather than do corrective actions on validation errors. This is the standard on how input fields behaves.

Although you introduce some code enhancements, I have the feeling that the new logic is a bit messy and results in more bugs. I will have to double check your final implementation after you fix the bugs.

- UPDATED: Enhanced min/max value validation to handle empty attributes correctly - UPDATED: Converted `validInput.min` and `validInput.max` to numbers for consistent comparisons - ADDED: Allow temporary entry of negative (`"-"`) and floating point (`"."`) characters before validation - FIXED: Prevent premature value correction while the user is still typing

mohamedhyouns · 2025-02-03T07:28:39Z

For ranges like [-100, 100] with default step, I cannot add negative values (it resets to mid range).

The slider supports negative value: 7a41514

Value resets are confusing: Sometimes reset to max (i.e., when exceed maximum), reset to mid (i.e., on invalid input or on clear).

I agree, but not in all mentioned cases.

reset to max (i.e., when exceed maximum) -> This is a MUST since the input is bound by the slider. Please take a look at this component.
Clear now is allowed: 4e88e93 and 09d663a

In the 100 step example, putting a fraction of a step value do approximation on the range field while keep the number as is in the input field. Did we decide at some point to do this approximation part? Regardless, there is a mismatch in behavior between the input and range fields.

~~Sorry but didn't understand your point.~~

I asked ChatGPT about the proper Rounding Formula, and it gave me this 🙂

Seems to work fine for me: 1354a0c

We should provide feedback rather than do corrective actions on validation errors. This is the standard on how input fields behaves.

The core implementation doesn't support this at the moment. Adding it would require a new issue, as it involves some complexity and is difficult to incorporate into the current atomic PR.

- ADDED: `roundToPrecision` function to maintain correct floating-point precision - UPDATED: Ensured number input allows empty values without applying calculations - UPDATED: Added explicit parsing for `min`, `max`, and `step` attributes to prevent unintended behavior - UPDATED: Clamped input values within the allowed range before applying rounding - UPDATED: Improved rounding logic to correctly align with step increments - FIXED: Ensured precision consistency when updating number input values

mohamedhyouns · 2025-07-16T03:40:29Z

Some references:

mohamedhyouns added 5 commits December 12, 2024 08:05

chore: generate the new assets

f16a7c1

refactor: add 'name' attributes for proxy-based synchronization

7c4183f

- Added `name` attributes to slider inputs in `index.html` for proxy-based synchronization.

fix: unit tests

6616d75

chore: deprecate multi-stage event test

6417327

Removed test as the proxy now handles input and range updates automatically.

mohamedhyouns mentioned this pull request Dec 12, 2024

Fix two way binding poc #31

Closed

mohamedhyouns added 2 commits December 12, 2024 08:53

test: add new test coverage

73174a8

style: reformating the file

c38fb11

The current style of file not consistent with vscode reformating style

mohamedhyouns requested a review from mhmelshaaerdxpr December 12, 2024 07:03

jjroelofs reviewed Dec 12, 2024

View reviewed changes

jjroelofs closed this Dec 12, 2024

mohamedhyouns reopened this Dec 16, 2024

mohamedhyouns mentioned this pull request Dec 16, 2024

Buggy behaviors in number input #32

Open

mohamedhyouns added 3 commits December 17, 2024 07:45

fix: replace name with data-dxb-proxy-key

71f288d

fix: set slider in the middle if the input field is cleared

8ecc562

fix: set slider in the middle in both inputs when input number is empty

402568a

mohamedhyouns requested a review from jjroelofs December 22, 2024 07:25

Merge branch '2.x' into mohamedhany/2.x/#32-refactor-two-way-binding

91878c7

jjroelofs requested changes Jan 7, 2025

View reviewed changes

mhmelshaaerdxpr suggested changes Jan 20, 2025

View reviewed changes

mohamedhyouns added 2 commits January 21, 2025 08:00

fix: prevent input value from exceeding maximum value

fd63495

fix: support new slider investment

fcc97f4

mhmelshaaerdxpr reviewed Jan 21, 2025

View reviewed changes

mohamedhyouns added 3 commits January 21, 2025 10:44

fix: deprecate data-dxb-proxy-key data attribute

5aa8e33

fix: MutationObserver infinite loop

68ee689

fix: move reset max logic to proxy

a4496c0

mohamedhyouns added 2 commits January 22, 2025 10:50

chore: decouple input listeners

a4a1d4c

chore: update dist file

c2927bf

mohamedhyouns requested a review from mhmelshaaerdxpr January 22, 2025 08:53

mhmelshaaerdxpr suggested changes Jan 28, 2025

View reviewed changes

mohamedhyouns added 4 commits February 3, 2025 09:09

chore: update tests

631254b

fix: allow empty/clear input

4e88e93

chore: update dist file

f789629

mohamedhyouns requested a review from mhmelshaaerdxpr February 3, 2025 07:29

mohamedhyouns and others added 4 commits February 3, 2025 09:31

chore: remove getDefaultValue

bba3606

fix: allow emptiness when clearing in case of min isn't Zero

09d663a

Merge branch '2.x' into mohamedhany/2.x/#32-refactor-two-way-binding

901089a

Conversation

mohamedhyouns commented Dec 12, 2024

Uh oh!

mohamedhyouns commented Dec 12, 2024

Uh oh!

mohamedhyouns commented Dec 12, 2024

Uh oh!

jjroelofs left a comment

Choose a reason for hiding this comment

Uh oh!

mohamedhyouns commented Dec 16, 2024

Uh oh!

mohamedhyouns commented Dec 22, 2024

Uh oh!

jjroelofs left a comment

Choose a reason for hiding this comment

Uh oh!

mhmelshaaerdxpr left a comment

Choose a reason for hiding this comment

Uh oh!

mhmelshaaerdxpr Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

mohamedhyouns Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

mhmelshaaerdxpr Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

mohamedhyouns Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

mhmelshaaerdxpr Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohamedhyouns Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

mhmelshaaerdxpr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohamedhyouns commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohamedhyouns commented Jul 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhmelshaaerdxpr Jan 21, 2025 •

edited

Loading

mhmelshaaerdxpr left a comment •

edited

Loading

mohamedhyouns commented Feb 3, 2025 •

edited

Loading