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

Remove checkbox/radio toggle from button plugin in favor of a CSS only solution #30650

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Apr 26, 2020

Alternative solution for #28463. This PR also separates the checkboxes docs from the button groups and buttons.

  • The Button plugin is removed in favor of a CSS only solution, a checkbox/radio with a .btn-toggle class. This solution works with as well normal as outlined buttons.
  • The checkbox and radio toggles docs are moved to the form checks docs.
  • An active state example for links is added to the button groups
  • Some outline examples are added to the button groups.

This PR removes the button toggle functionality (toggling active classes to buttons). I think it's better to just rely on checkboxes to toggle states, since the the js solution introduces issues like #25122.

Note the increase of CSS file size will be negligible once we use custom properties for our button component.

Checks:

  • Check for .btn-group-toggle leftovers in docs.
  • Check for data-toggle="button" leftovers in docs.

Closes #28463
Closes #30615
Closes #25122 since the button plugin is removed

@MartijnCuppens MartijnCuppens added this to Inbox in v5 via automation Apr 26, 2020
@MartijnCuppens MartijnCuppens marked this pull request as ready for review April 26, 2020 14:22
@MartijnCuppens MartijnCuppens requested review from a team as code owners April 26, 2020 14:22
@mdo
Copy link
Member

mdo commented Apr 26, 2020

I haven’t reviewed on my Mac, but on iOS the outline buttons at https://deploy-preview-30650--twbs-bootstrap.netlify.app/docs/4.3/forms/checks/#toggle-buttons don’t seem to work right. I can tap to check the first two, but unchecking seems busted. Could this be a stuck focus state that’s too similar to the active state?

@mdo
Copy link
Member

mdo commented Apr 26, 2020

Also love this PR in general!

@MartijnCuppens
Copy link
Member Author

Could this be a stuck focus state that’s too similar to the active state?

The hover and focus state of outline buttons have a solid background, that's why there's no change. We could continue with #28999, which has alternative hover styles and would also fix this issue

@XhmikosR
Copy link
Member

XhmikosR commented Apr 27, 2020

I'm not sure this should be in v5. It should probably be in a next release alpha.

Also, traditionally, we promote semantic HTML which is what a button is for?

Regardless, we shouldn't hurry with this change since it's quite a radical one IMHO.

@MartijnCuppens
Copy link
Member Author

Also, traditionally, we promote semantic HTML which is what a button is for?

AFAICT, an element which indicates a boolean state should be a checkbox, not a button with an active class (vice versa for the radios).

we shouldn't hurry with this change since it's quite a radical one IMHO

#28463 was on the v5 board and @patrickhlauke bumped us a lot to continue with this. Imo we shouldn't re-postpone this once again.

@patrickhlauke
Copy link
Member

not had a chance to fully review, but based on the conversation here:

  • i'm keen to finally drop all the horrible js noodlings we do for faked checkboxes/radio buttons - it's the one aspect of bootstrap that has continued to cause problems (making it work with keyboard/mouse consistently). it's an arcane approach, and it's a problem that has been elegantly solved ages ago with stuff like wtfforms.com. i would be very annoyed if for v5 we still used the old hacky flaky JS-based method.
  • there IS still a need for <button aria-pressed="...">. the semantics (from an assistive technology perspective) are different between a checkbox (checked/unchecked) and a button (pressed/not pressed). yes, if you squint, you can argue that a pressed/unpressed button is like a checked/unchecked checkbox, but the difference in how they're announced can be confusing. we can make the distinction in documentation (happy to write something up at some point), but i would not say we can just drop the aria-pressed part of the button plugin. i would NOT want to see this dropped in v5

@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Apr 27, 2020

but the difference in how they're announced can be confusing

Ok, makes sense. I'll revert the change to the progress bar animation toggle button in our docs then.

i would not say we can just drop the aria-pressed part of the button plugin. i would NOT want to see this dropped in v5

So if I understand that correctly, what you want to keep is basically this: https://codepen.io/MartijnCuppens/pen/yLYbWwp

If so, I'm not sure what the purpose is of having such a small plugin is. Developers will still need to write their custom js to interact with these buttons in the end. We also have documented the need of the aria-pressed attribute here https://twbs-bootstrap.netlify.app/docs/4.3/components/buttons/#active-state?

@twbs/team, any strong opinions on this? Should we keep the keep the button plugin only to bind the active class toggle to the aria-pressed state?

@mdo
Copy link
Member

mdo commented Apr 30, 2020

With such a short snippet of JS and a single behavior required for JS, this makes me think we should have some place to callout common snippets for folks. We've had to do this for custom file input and custom form validation behavior. Could this be something we maintain in our docs as a separate page under say Helpers as docs/helpers/javascript?

@ffoodd
Copy link
Member

ffoodd commented Apr 30, 2020

@mdo I love the idea. However be conscious that we'll have tons of questions about those snippets: we'd need to add reference links for edge cases, I guess.

@XhmikosR
Copy link
Member

IMHO that's a step backwards. We shouldn't ask people to add things manually, it's not user-friendly and most people want and value something that works out of the box.

@MartijnCuppens
Copy link
Member Author

I was updating the docs with the snippet and I got to admit it looked pretty user-unfriendly. I'll just restore the button toggle and update the migration guide so that it's clear only the checkbox hacks were removed from the js plugin.

@MartijnCuppens MartijnCuppens changed the title Drop button plugin in favor of CSS only solution Remove checkbox/radio toggle from button plugin in favor of a CSS only solution May 2, 2020
@MartijnCuppens MartijnCuppens force-pushed the master-mc-button-toggles branch 2 times, most recently from 430f365 to 916317b Compare May 2, 2020 09:18
@MartijnCuppens
Copy link
Member Author

Restored the button plugin (to toggle active classes) and clarified this PR title. Let the reviews rip!

@patrickhlauke
Copy link
Member

Button plugin
Do more with buttons. Control button states or create groups of buttons for more components like toolbars.

the idea that the plugin allows/does anything for groups of buttons should probably be removed. In my #28463 i neutered it to just say (https://deploy-preview-28463--twbs-bootstrap.netlify.app/docs/4.3/components/buttons/#button-plugin)

Button plugin
The button plugin allows you to create simple on/off toggle buttons.

I'd also be keen to see the button group examples expanded to show button groups with checkboxes/radio buttons https://deploy-preview-28463--twbs-bootstrap.netlify.app/docs/4.3/components/button-group/#checkbox-and-radio-button-groups and the added vertical variation example with radio buttons https://deploy-preview-28463--twbs-bootstrap.netlify.app/docs/4.3/components/button-group/#vertical-variation ... but I think disagreement on the best way forward on those is what stalled #28463 at the time

@patrickhlauke
Copy link
Member

also, maybe not here, but have to say i still find the lack of clear differentiation (for buttons and button groups) between focus, active, focus+active very hard to visually keep apart

scss/_button-group.scss Show resolved Hide resolved
@@ -209,3 +209,54 @@ Omit the wrapping `.form-check` for checkboxes and radios that have no label tex
<input class="form-check-input" type="radio" name="radioNoLabel" id="radioNoLabel1" value="" aria-label="...">
</div>
{{< /example >}}

## Toggle buttons
Copy link
Member

Choose a reason for hiding this comment

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

We call these toggle buttons here, but the language used on the radio/checkbox base classes has been .form-check. Maybe we keep the heading here as Toggle buttons, but swap the class for .btn-check? Caught myself getting confused too because of .dropdown-toggle being the class you interact wiht in dropdowns.

@MartijnCuppens
Copy link
Member Author

Sorry for the delay here. Rebased the PR and tackled the feedback from @mdo and @patrickhlauke:

the idea that the plugin allows/does anything for groups of buttons should probably be removed

Changed that.

I'd also be keen to see the button group examples expanded to show button groups with checkboxes/radio buttons

Imo, this is not blocking issue for this PR, but definitely worth looking into later on.

also, maybe not here, but have to say i still find the lack of clear differentiation (for buttons and button groups) between focus, active, focus+active very hard to visually keep apart

See #28999.

Maybe we keep the heading here as Toggle buttons, but swap the class for .btn-check?

Yup, changed that.

@mdo mdo moved this from Inbox to Review in v5 Jun 15, 2020
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Huge lift and update for v5, thank you @MartijnCuppens!

v5 automation moved this from Review to Approved Jun 16, 2020
@mdo mdo merged commit 93f1028 into master Jun 16, 2020
v5 automation moved this from Approved to Shipped Jun 16, 2020
@mdo mdo deleted the master-mc-button-toggles branch June 16, 2020 02:04
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Aug 31, 2021
Bootstrap 5 dropped the support for data-bs-toggle="buttons".
See: twbs/bootstrap#30650
The replacement is to use the class "btn-check" in combination with the
state of input fields. This has been done in the Localization module.
Some other occurrences didn't rely on the bootstrap js and it was
enough to simply remove the data attribute.

Also remove a forgotten console.log debug.

Resolves: #95048
Releases: master
Change-Id: Icd31e7cc0cb9eb1581a7bcfe1448ed7dad24a935
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/70832
Tested-by: Oliver Bartsch <[email protected]>
Tested-by: core-ci <[email protected]>
Tested-by: Benni Mack <[email protected]>
Reviewed-by: Oliver Bartsch <[email protected]>
Reviewed-by: Benni Mack <[email protected]>
TYPO3IncTeam pushed a commit to TYPO3-CMS/backend that referenced this pull request Aug 31, 2021
Bootstrap 5 dropped the support for data-bs-toggle="buttons".
See: twbs/bootstrap#30650
The replacement is to use the class "btn-check" in combination with the
state of input fields. This has been done in the Localization module.
Some other occurrences didn't rely on the bootstrap js and it was
enough to simply remove the data attribute.

Also remove a forgotten console.log debug.

Resolves: #95048
Releases: master
Change-Id: Icd31e7cc0cb9eb1581a7bcfe1448ed7dad24a935
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/70832
Tested-by: Oliver Bartsch <[email protected]>
Tested-by: core-ci <[email protected]>
Tested-by: Benni Mack <[email protected]>
Reviewed-by: Oliver Bartsch <[email protected]>
Reviewed-by: Benni Mack <[email protected]>
TYPO3IncTeam pushed a commit to TYPO3-CMS/install that referenced this pull request Aug 31, 2021
Bootstrap 5 dropped the support for data-bs-toggle="buttons".
See: twbs/bootstrap#30650
The replacement is to use the class "btn-check" in combination with the
state of input fields. This has been done in the Localization module.
Some other occurrences didn't rely on the bootstrap js and it was
enough to simply remove the data attribute.

Also remove a forgotten console.log debug.

Resolves: #95048
Releases: master
Change-Id: Icd31e7cc0cb9eb1581a7bcfe1448ed7dad24a935
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/70832
Tested-by: Oliver Bartsch <[email protected]>
Tested-by: core-ci <[email protected]>
Tested-by: Benni Mack <[email protected]>
Reviewed-by: Oliver Bartsch <[email protected]>
Reviewed-by: Benni Mack <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5
  
Shipped
5 participants