Skip to content

Conversation

@smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Oct 12, 2024

Why is it needed?

For some TVs, the switch style may be more appropriate to the content being collected. This update allows users to choose how to display their checkbox TVs.

How to test

Create a new checkbox TV (or use an existing one) and verify that changing the "Display as Switch" option has the expected effect when the TV is displayed in the Resource editing form.

Related issue(s)/PR(s)

Alternate solution to PR #16623

Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the quick work!

Copy link
Member

@opengeek opengeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will merge #16623 for 3.0.6 to restore consistency there. Following that path, we should make this solution default to toggle to be consistent with 3.0.x.

@opengeek opengeek added this to the v3.1.0 milestone Oct 16, 2024
@smg6511
Copy link
Collaborator Author

smg6511 commented Oct 16, 2024

@opengeek The established default has been to show a traditional checkbox. In the initial (alternative) PR referenced above, the contributor said that the checkboxes appeared as toggles in the original 3.0.0 release. Not to say that absolutely was not the case, but I couldn't find evidence backing that up in the blame for the couple of relevant render tpl files. For a time I believe they did render as toggles (in the dev branch) as we were working up to the release; and I remember a lot of back and forth over the use of the toggle styling in general (particularly with Ruslan).

If that styling did, in fact, make it into 3.0.0, we subsequently and purposely reversed course (seeing it as a mistake of sorts).

Other thing is that it's a simpler move for this new setting to default to false. Defaulting to true would at least require a non-ideal conditional (if the prop doesn't exist OR it does and is true, show as toggle) and more invasively (and probably more proper) would involve an update script to add the setting to all existing checkbox TVs in the db.

I'm happy to make the change, but you may not want it given what I've said above.

@opengeek
Copy link
Member

@smg6511 I see changes to the CSS related to the display-switch class, and I'm wondering if this is where the inconsistency was introduced after 3.0.1. Can you take a look at the history there in _forms.scss?

@smg6511
Copy link
Collaborator Author

smg6511 commented Oct 22, 2024

@opengeek - Ahh, yes, ok - that refreshes my memory re what happened: At first, the style (without a special class) was applied in a way that affected most of the Resource editing form panel. I say most because it did not apply to some areas, like the Resource Groups tab where regular checkboxes still appeared. We later backed out of that, realizing we only really wanted the switch to appear on the UI toggles and not all UI checkboxes ... and also not on TV checkboxes. (I didn't remember this ending up in production, but maybe it did after all.) Anyway, we soon after corrected this by adding the 'display-switch' class and applying it only in the intended areas.

@opengeek opengeek merged commit e3b6da2 into modxcms:3.x Oct 24, 2024
jenswittmann pushed a commit to jenswittmann/revolution that referenced this pull request Dec 23, 2024
…odxcms#16631)

### Why is it needed?
For some TVs, the switch style may be more appropriate to the content
being collected. This update allows users to choose how to display their
checkbox TVs.

### How to test
Create a new checkbox TV (or use an existing one) and verify that
changing the "Display as Switch" option has the expected effect when the
TV is displayed in the Resource editing form.

### Related issue(s)/PR(s)
Alternate solution to PR modxcms#16623
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.

3 participants