modxcms
Adding missing class for checkbox TVs
What does it do?
Fixes regression bug when new display-switch class was introduced. At least, if was not meant to be introduced.
I upgraded from MODX 3.0.1 to 3.0.5 and noticed the changes.
Before 3.0.1
After 3.0.5
Why is it needed?
Checkboxes were no longer being rendered as switches / toggles
How to test
For example: add a MODX TV via Form Customisation to modx-resource-main-right-top region
@sonicpunk Thank you for jumping in here to make a contribution!
I think we're going to want to go about this in a different way, as the switch style isn't appropriate for a lot of checkbox-type inputs. It signals more of a setting (i.e., something to turn on and off). The way to go I think is to offer an option at the checkbox TV creation stage that allows people to "Use Switch Style" on a TV-by-TV basis. I'm the one who implemented the TV creation/editing refactor for 3.0, so I can help if needed; or you can take a stab at it ;-)
@Ibochkarev Ok, that is also an idea. I was just focused on the visual regression. I am not sure if I will be able to dig deeper into this at the moment. Any help from you would be greatly appreciated!
Thanks for the PR. What's the point of toggles? If only for beauty, then the idea is so-so. Firstly, checkboxes are simpler and clearer, secondly, now in MODX3 checkboxes are already in 2 variants, which looks strange. And making a whole setting in TV for visual design is not a good solution.
For me the point is to have a consistent user interface. If it was decided that for MODX 3, checkboxes should look like toggles, than there should not be variations, IMHO. But if there are valid use cases for different variations, they should be discussed. In our case, we are using TVs for custom resource settings, and find the toggle UI to have a modern feel and is consistent with how other Resource settings work, such as Publishing.
Looks solid to me. I think it should be consistent with the earlier release.
@Ruslan-Aleev @sonicpunk - In my mind there are two distinct functionalities of a checkbox field. (Take the fact that the input type is checkbox out of the equation and just think of what it's being used for.) On the one side there is a switch-like field: a field that turns something on/off. On the other, there's a field that's part of a group of checkbox fields having the purpose of selecting a number of items that represent a collection of some sort. If you agree with that line of reasoning, there is, in fact, justification to have two distinct checkbox field styles.
It's really not all that unlike the fact that we can collect an array of selections with either a select (multi) or checkbox fields; they do the same thing but have a completely different visual presentation.
That said, I still think this should be a TV creation option and not an across-the-board style. One thing that might help if we went in that direction would be to give our standard checkbox an updated look as, right now, they're the one thing in the interface that looks like it's stuck in the 80s (ok, maybe 90s) ;-)
@sonicpunk @smg6511 I'm with Jim on this one. Toggles are not 1:1 replacement for checkboxes, they are two distinct UI elements.
I've created several apps where I needed both, for the exact same reason Jim shared.
I'd vote for adding an input option that would render the checkbox as a toggle.
@theboxer @smg6511 To have a visual representation option would be great. My concern here in the PR was just keeping the consistency that was previously there.
@sonicpunk That's fair :) I'd take it as an opportunity to make it work properly, instead of reverting to the previous state which IMHO wasn't good.
Seems like @smg6511 should be able to help with adding the input option for this.
That's purely because this targets the 3.x branch (3.1 release).
I'm not sure how I feel about the inconsistency between 3.0.1 and 3.0.5, maybe this PR should target 3.0.x branch and bring the consistency back, while adding the input option for 3.1? I'm torn on this approach just because I don't like toggles to be default for all checkboxes....
@theboxer The option isn't hard to implement; I could put that together by this weekend. BTW, I had difficulty finding the switch being implemented in 3.0 (looking at the blame for the render tpl), although perhaps it was done in a different way.
Given the new https://github.com/modxcms/revolution/pull/16631 I think this could probably be closed at this point as the new PR allows for both options.
Upon review of #16631, I feel it does appear to correctly address this issue going forward. Of course it comes at a cost for those who had switch TVs and now have checkboxes as it will require people to change their TV settings for the checkbox TVs. My assumption (yeah, I know the cliché), is that this allows folks to get the asthetic back for their TVs but not affecting the defaults for all future TV creation.
I could be convinced in #16631 that a system setting could be created to set the default behavior but I am about a 2/10 on the care scale.
We would include a note in the release notes about the change.
To be clear I am recommending closing this one in favour of #16631.
