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

[5.x] Fix ButtonGroup not showing active state if value are numbers #10916

Open
wants to merge 8 commits into
base: 5.x
Choose a base branch
from

Conversation

morhi
Copy link
Contributor

@morhi morhi commented Oct 8, 2024

Due to a change in 0b85403 the ButtonGroup fieldtype cannot display a button as active, if the key is a number. This is because the HTML button uses strings for its value, but the PHP side will cast numeric values to numbers, which then will fail the === comparison.

Also, if you change the default options for the Width fieldtype the resulting list will contain integer and string keys. It breaks the Width fieldtype to properly display the selected width, because it uses a strict comparison, too.

Since all HTML form elements like select, input, button work with strings only, I thought it makes sense, to work with string values on the JS side only, too. The Width fieldtype needs to compare numbers, so it will cast all options to a number and ignore non-numeric values, which don't really make sense in this field.

This change should not affect the augmented value of the fieldtypes, because during augmentation numeric values will be cast to an integer anyway.

This commit will

  • make the ButtonGroup fieldtype work with number keys again
  • make the Width fieldtype work again, if the default options have been changed by the user, because changing the prefilled list will result in a mix of strings and integers
  • output labels consistently as strings

Fixes #10915.
Fixes #11029.

morhi added 3 commits October 8, 2024 14:31
Due to a change in 0b85403 the
ButtonGroup fieldtype cannot display a button as active, if the
key is a number.

This commit will
- make the ButtonGroup fieldtype work with number keys again
- make the Width fieldtype work again, if the default options have been
  changed by the user, because changing the prefilled list will result
  in a mix of strings and integers
- output labels consistently as strings
@jasonvarga
Copy link
Member

I appreciate all the work that went into this PR. Thank you.


Due to a change in 0b85403 the ButtonGroup fieldtype cannot display a button as active, if the key is a number. This is because the HTML button uses strings for its value, but the PHP side will cast numeric values to numbers, which then will fail the === comparison.

Why can't we just make it a loose comparison in the JS instead?

:class="{'active': value === option.value}"

Changing this to a loose comparison seems to solve the problem without any PHP changes needed.


Also, if you change the default options for the Width fieldtype the resulting list will contain integer and string keys. It breaks the Width fieldtype to properly display the selected width, because it uses a strict comparison, too.

It seems to work fine for me. It already uses a loose comparison.

{ 'filled': selected >= width, 'selected': selected == width }

@morhi morhi force-pushed the fix-button-group-integer-handling branch from 4cb349e to ed47283 Compare November 5, 2024 11:29
@morhi
Copy link
Contributor Author

morhi commented Nov 5, 2024

I appreciate all the work that went into this PR. Thank you.

Due to a change in 0b85403 the ButtonGroup fieldtype cannot display a button as active, if the key is a number. This is because the HTML button uses strings for its value, but the PHP side will cast numeric values to numbers, which then will fail the === comparison.

Why can't we just make it a loose comparison in the JS instead?

:class="{'active': value === option.value}"

Changing this to a loose comparison seems to solve the problem without any PHP changes needed.

I thought about changing these types to ensure a better consistency. The PHP change actually affects the label type only, enforcing it to be a string always, but maybe it is better to leave it untouched. I changed it back, leaving the Unit Tests though to also test the return types for values and labels, if the source values are numbers. I tested the code without enforcing string value within HasInputOptions.js and changed the comparison to a loose comparison. I realized though, that in this case the update handler should use the original option value, instead of the $event.target.value, so that the update event always uses the same type as the :value binding. Otherwise, numbers would be casted to strings at this place. However, this would also store the value as numbers in the flat files, which could lead to unexpected behaviour, if someone checks the raw type of the entry value. The augmented/computed value still returns a number though.

Also, if you change the default options for the Width fieldtype the resulting list will contain integer and string keys. It breaks the Width fieldtype to properly display the selected width, because it uses a strict comparison, too.

It seems to work fine for me. It already uses a loose comparison.

{ 'filled': selected >= width, 'selected': selected == width }

Yea, it works :)

@morhi morhi changed the title [5.x] Fix handling number keys in fieldtypes using HasSelectOptions [5.x] Fix ButtonGroup not showing active state if value are numbers Nov 5, 2024
@morhi
Copy link
Contributor Author

morhi commented Dec 10, 2024

This PR is ready for review, despite my long previous commit, just in case you got confused by it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants