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

Adding missing class for checkbox TVs #16623

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

sonicpunk
Copy link

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
Screenshot 2024-09-27 at 16 41 51

After 3.0.5
Screenshot 2024-09-27 at 16 41 20

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

Fixes regression bug when new display-switch class was introduced.
@smg6511
Copy link
Collaborator

smg6511 commented Sep 27, 2024

@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 ;-)

@sonicpunk
Copy link
Author

@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!

@Ruslan-Aleev
Copy link
Collaborator

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.

@sonicpunk
Copy link
Author

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.

@rthrash
Copy link
Member

rthrash commented Oct 4, 2024

Looks solid to me. I think it should be consistent with the earlier release.

@smg6511
Copy link
Collaborator

smg6511 commented Oct 4, 2024

@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) ;-)

@rthrash rthrash added this to the v3.1.0 milestone Oct 9, 2024
@theboxer
Copy link
Member

theboxer commented Oct 10, 2024

@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.

@sonicpunk
Copy link
Author

@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.

@theboxer
Copy link
Member

@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.

@theboxer
Copy link
Member

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....

@smg6511
Copy link
Collaborator

smg6511 commented Oct 10, 2024

@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.

@opengeek opengeek modified the milestones: v3.1.0, v3.0.6 Oct 16, 2024
@opengeek opengeek changed the base branch from 3.x to 3.0.x October 16, 2024 14:20
@opengeek opengeek changed the base branch from 3.0.x to 3.x October 16, 2024 14:21
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.

7 participants