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

FIX difference in behaviour between mouse & keyboard interactions on Sliders #12378

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

rickcurran
Copy link
Contributor

Description

This fix is to resolve an issue that I encountered when using a particular combination of a Slider with a bound input field, in this case the bound input field has a currency-formatting function applied on the moved.zf.slider' event, this function renders the value in the bound input field as a currency value e.g. £100,000, (See original discussion post: https://github.com/foundation/foundation-sites/discussions/12372).

See example CodePen showing the issue:
https://codepen.io/rickcurran/pen/WNZqPyo

The issue encountered is that when using mouse interaction the functionality worked correctly, but moving the same slider using the keyboard arrows resulted in a NaN error in the bound input field.

Looking at the code that deals with the movement of the slider handles in foundation.slider.js, when the handles are moved via the mouse it gets the value of the slider's position from a data-attribute on the slider aria-valuenow, but in the code that handles the keyboard arrow movement it was getting the value directly from the related bound input field instead.

So the issue with the keyboard usage is that when it gets the value from the input field which contains a currency string like £100,000 rather than plain 100000, this results in an NaN error when parseFloat iss applied to it.

Here's the line that gets the value for mouse interaction:

var h1Val = parseFloat(this.$handle.attr('aria-valuenow'));

Here's the line that gets the value for keyboard interaction:

oldValue = parseFloat(_this.inputs.eq(idx).val()),

So my proposed fix is to make the keyboard interaction get the value from the same data-attribute, so line 524 would be this instead:

oldValue = parseFloat($handle.attr('aria-valuenow')),

Types of changes

  • Documentation
  • [X ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (anything that would change an existing functionality)
  • Maintenance (refactor, code cleaning, development tools...)

Checklist

  • [ X] I have read and follow the CONTRIBUTING.md document.
  • [ X] The pull request title and template are correctly filled.
  • [ X] The pull request targets the right branch (develop or develop-v...).
  • [ X] My commits are correctly titled and contain all relevant information.
  • [] I have updated the documentation accordingly to my changes (if relevant).
  • [] I have added tests to cover my changes (if relevant).

…ns on Sliders

This fix is to resolve an issue that I encountered when using a currency-formatted value in the bound input on a slider (discussion post: https://github.com/foundation/foundation-sites/discussions/12372).

When using mouse interaction the functionality worked correctly, but moving the same slider resulted in a `NaN` error in the bound input field.

Looking at the code that deals with the movement of the slider handles, when the handles are moved it gets the value of the slider's position from a data-attribute on the slider aria-valuenow, but in the code that handles the keyboard arrow movement it was getting the value directly from the related bound input field instead.

This change modifies the code so that it gets the value from the same data-attribute for keyboard interaction as well.
@sonarcloud
Copy link

sonarcloud bot commented Jan 27, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@joeworkman joeworkman merged commit 37ba262 into foundation:develop Jul 11, 2022
@joeworkman
Copy link
Member

closes #12379

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

2 participants