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

[4.0] Revert switcher #15946

Closed
wants to merge 9 commits into from
Closed

[4.0] Revert switcher #15946

wants to merge 9 commits into from

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented May 10, 2017

Pull Request for Issue #15391 .

Summary of Changes

  • Changes the order of the options back to what we have in J3, meaning Yes/Show on left side and No/Hide on the right.
  • Removes the "Switcher" layout and JS and restores the btn-group behavior we have in J3.

Testing Instructions

Open various settings page. Most occurances of those radio elements are typical in component settings.

Expected result

Have an intuitive toggle button

Actual result

Have an unintuitive switcher thing with inverted option order and unclear states

Documentation Changes Required

None

Known issue

Maybe someone can help me here:
The btn-group doesn't work with the <fieldset> tag. I changed the tag generated in the radio JLayout to <div> and it works fine. This may be a bug in BS4, I wasn't able to figure out where the difference comes from.

@Bakual Bakual changed the title Revert switcher [4.0] Revert switcher May 10, 2017
@dgrammatiko
Copy link
Contributor

@Bakual you need to remove switcher from grunt-settings.yaml and remove the related lines from Gruntfile.js.

The btn-group doesn't work with the

tag. I changed the tag generated in the radio JLayout to
and it works fine. This may be a bug in BS4, I wasn't able to figure out where the difference comes from.

Just rename the <div> to <joomla-buttons> and use the related custom element 😀

Anyways I'm not in favour of removing the switcher but I'm ok with reverting the xml changes. By the way switcher can still be done with the original xml structure...

I think this should be a temporary fix until we can restore the fieldset tag.
@Bakual
Copy link
Contributor Author

Bakual commented May 10, 2017

you need to remove switcher from grunt-settings.yaml and remove the related lines from Gruntfile.js.

@dgt41 Afaik it wasn't a file pulled in using Grunt, it something done ourself. I did have a look now at those grunt files and couldn't find anything related to switcher.js. If I missed something, feel free to point me to it.

Anyways I'm not in favour of removing the switcher but I'm ok with reverting the xml changes. By the way switcher can still be done with the original xml structure...

I did the changes to the XML, JS and CSS in separate commits for that reason so it would be easy to take only parts of this PR.
I think the switcher needs some serious work anyway if you want to use that. It lacks in many aspects currently. It may have situational use cases, but for regular options in components settings or menu params and the like I don't see us using it. It doesn't even look nice between selects and other inputs. The style and width just don't match.

@brianteeman
Copy link
Contributor

My 2c i am not convinced that a switcher is suitable for a desktop application - so at this time I would agree to removing it and reverting back as here. It can be looked at again in the future

@mbabker
Copy link
Contributor

mbabker commented May 10, 2017

So this decision was actually driven based on us having some conflicts between CSS frameworks, but we ended up pulling out all of the switcher based checkboxes in the frontend of an app we're building right now in favor of "plain" checkboxes. I realize our switchers are primarily radio based, but the same general concept applies.

screen shot 2017-05-10 at 1 53 18 pm

screen shot 2017-05-10 at 1 53 34 pm

@Bakual
Copy link
Contributor Author

Bakual commented May 10, 2017

Got the btn-group-yesno working now. Still need to figure out the fieldsetvs div thing. I'm still lost there 😄

@mbabker mbabker added this to Testing/Review in [4.0] General May 15, 2017
@brianteeman brianteeman modified the milestone: Joomla 4.0 Jun 8, 2017
@brianteeman
Copy link
Contributor

At this time I am closing this PR. We need to let the J4 guys deliver and alpha with this and then we can of course review it then when a wider audience have seen it.

Change is always disturbing but we have to be brave and try things.

@mbabker mbabker removed this from the Joomla 4.0 milestone Jul 12, 2017
@mbabker mbabker removed this from Testing/Review in [4.0] General Jul 12, 2017
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

5 participants