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

v4.2: switch custom form control #23004

Merged
merged 15 commits into from
Dec 12, 2018

Conversation

gijsbotje
Copy link
Contributor

@gijsbotje gijsbotje commented Jul 6, 2017

This adds a neat styling feature for the custom form controls.
Markup stays the same as the checkbox but displays it as a toggle switch.

schermafbeelding 2017-07-06 om 11 25 55

https://codepen.io/gijsbotje/pen/rwKMmy

https://deploy-preview-23004--twbs-bootstrap4.netlify.com/docs/4.1/components/forms/#switches

@gijsbotje gijsbotje changed the title switch custom form contrl switch custom form control Jul 6, 2017
docs/4.0/components/forms.md Outdated Show resolved Hide resolved
@gijsbotje
Copy link
Contributor Author

gijsbotje commented Jul 6, 2017

fixes a part of #22090

@mdo
Copy link
Member

mdo commented Jul 6, 2017

Dope! Thank you for jumping in on this. I'll take a look at it for our v4.1 ship :).

@gijsbotje gijsbotje changed the title switch custom form control v4.1: switch custom form control Jul 11, 2017
@gijsbotje
Copy link
Contributor Author

gijsbotje commented Dec 31, 2017

@mdo @XhmikosR @andresgalante I messed up here :( and cant figure out how to revert it, any suggestions?
Sorry for this... don't understand what wen't wrong

-----edit-----
restored most of it, just the bootstrap-reboot.css file that's not working out

@XhmikosR
Copy link
Member

@gijsbotje: you can just do git rebase -i upstream/v4-dev and clean up your local branch.

@gijsbotje
Copy link
Contributor Author

maybe i did something wrong, but it didn't work...

@XhmikosR XhmikosR force-pushed the custom-switch-form-control branch from 3f3fd85 to 0814b27 Compare December 31, 2017 14:41
@XhmikosR
Copy link
Member

@gijsbotje: you had messed up your branch. Do not merge from upstream. Just rebase it, if needed.

Now, I cleaned it up and force pushed it. So, make sure you do git fetch origin && git reset --hard origin/custom-switch-form-control before anything else otherwise you will overwrite the cleanup.

@gijsbotje
Copy link
Contributor Author

@XhmikosR thanks dude, will do
i tried to make a pr for a different issue and my forked v4-dev branch is messed up i think
i'll see if i can fix it all

@m5o
Copy link
Contributor

m5o commented Feb 27, 2018

@gijsbotje / @mdo the solution to modifying the custom-checkbox with .custom-checkbox-switch is super handy.
http://wingman.mediumra.re/components-bootstrap.html#forms

docs/4.0/components/forms.md Outdated Show resolved Hide resolved
@gijsbotje gijsbotje changed the title v4.1: switch custom form control v4.2: switch custom form control Apr 12, 2018
@alecpl alecpl mentioned this pull request May 12, 2018
Copy link

@ayoubkhan558-zz ayoubkhan558-zz 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 it should be added to bootstrap, so that we don't need any external library for this purpose.

Copy link

@ayoubkhan558-zz ayoubkhan558-zz left a comment

Choose a reason for hiding this comment

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

Its a good option to have

@claudioalmeiida
Copy link

claudioalmeiida commented Oct 19, 2018

"mdo removed this from Inbox in v4.2" :(

@XhmikosR
Copy link
Member

Yeah, that was the issue I noticed too when I said I didn't think this was ready along with transitioning costly properties.

I'll try to have a look tomorrow, but @MartijnCuppens will probably beat me to it.

@gijsbotje
Copy link
Contributor Author

gijsbotje commented Nov 28, 2018

@XhmikosR @MartijnCuppens would this be ok? i used the color of the border for the indicator
image

@rafalp
Copy link

rafalp commented Nov 28, 2018

Can you include screenshot of other special control (eg. checkbox) being on/off/disabled, for comparison?

Also, is there "disabled + on" state?

@gijsbotje
Copy link
Contributor Author

@rafalp i've paired the switch with its checkbox equivalent
image

thanks for reminding me about the :checked:disabled state, forgot about that

- changed the indicator to not use the an svg
- changed animation to use transform
- tweaked height and with calculations of the indicator
- added disabled+checked support
- tweaked scss markup so it's the same as customo radio and checkbox
@gijsbotje
Copy link
Contributor Author

@MartijnCuppens fixed your comments as best as I could think of.
main question to resolve is if the color for the circle when it's unchecked is ok

@rafalp
Copy link

rafalp commented Nov 28, 2018

Those are looking very good, thanks 👍

@MartijnCuppens MartijnCuppens dismissed their stale review November 30, 2018 16:57

Changes applied

scss/_custom-forms.scss Show resolved Hide resolved
scss/_custom-forms.scss Outdated Show resolved Hide resolved
@MartijnCuppens
Copy link
Member

MartijnCuppens commented Nov 30, 2018

@gijsbotje, the color of the circle looks ok.
IMO the switch looks a bit small, maybe we should try to increase the size a bit? (see #23004 (comment))
It would also be nice to have total control over the track width and height and the spacing between the switch and the label text. With 'the track', I mean this element:
image

Maybe it's possible to tweak this a bit to so that it's possible to set the following variables:

  • The track width
  • The track height
  • Margin between the switch and the label text
  • The size of the circle

If we have control over this in _variables.scss, it would be easier to control the look of the switch and it would also be possible to make them look like in this PR:
#26691

@rafalp
Copy link

rafalp commented Nov 30, 2018

I would be careful with making switch too big - you'll potentially end with component being too big to use "inline", requiring special positioning or, worse, custom form layout altogether.

@MartijnCuppens
Copy link
Member

Good point, @rafalp. Forget about size increment than.

@rafalp
Copy link

rafalp commented Dec 1, 2018

Yeah, I am thinking now how this component should be sized. Right now its sized same way checkbox is, meaning its interchangeable with it. Perhaps it should also support .form-control-lg and .form-control-sm for extra variety?

Just dropping ideas - it may be too late for this, or material for new PR bumping extra sizing to custom controls?

@m5o
Copy link
Contributor

m5o commented Dec 2, 2018

💡 Support for contextual classes could be interesting, too.

@gijsbotje
Copy link
Contributor Author

gijsbotje commented Dec 3, 2018

💡 Support for contextual classes could be interesting, too.

@m5o I would do that as a seperate PR as it also affects the other controls

Yeah, I am thinking now how this component should be sized. Right now its sized same way checkbox is, meaning its interchangeable with it. Perhaps it should also support .form-control-lg and .form-control-sm for extra variety?

Just dropping ideas - it may be too late for this, or material for new PR bumping extra sizing to custom controls?

@rafalp Indeed as you said yourself material for new PR 😄

Maybe it's possible to tweak this a bit to so that it's possible to set the following variables:

  • The track width
  • The track height
  • Margin between the switch and the label text
  • The size of the circle
    If we have control over this in _variables.scss, it would be easier to control the look of the switch and it would also be possible to make them look like in this PR:
    One more custom checkbox switch, but more flexible #26691

@MartijnCuppens I was fiddling around with the variable custom-control-gutter, but this isn't really the gutter, right? It doesn't define the space between the indicator and the label, but rather defines the space that the label is pushed over. Then the indicator is pulled back the same amount.
So by default the gutter is set to 1.5rem. This gives the checkbox and radio a breathing room of .5rem. Therefore shouldn't gutter be .5rem? And the left properties set to $custom-control-indicator-size + $custom-control-gutter which would result in 1.5rem?

If we implement this change, controlling the track width and space between switch and the label would be much easier. to get it to be accurate in the current setup i would need to do something like:
$custom-switch-width + ($custom-control-gutter - $custom-control-indicator-size)
So if you guys agree with the change to $custom-control-gutter I will make a separate PR with those changes. I can imagine this would have some impact on people that make use of the sass, so that would be your call.

What i'm really struggling with and get wrap my head around is making the height and circle size customizable. I'm currently using the calc css function to get the height and with like this:
calc(#{$custom-control-indicator-size} - #{$custom-control-indicator-border-width * 4})
First the 1rem indicator-size then subtracting the border width * 4 to create space between the circle and the track. This can not be done in sass math as $custom-control-indicator-border-width is a pixel value and $custom-control-indicator-size is a rem value, which are incompatible in sass.

i'm also struggling with the positioning of the circle when you increase or decrease the size of the circle. currently this calc function is taking care of the vertical positioning:
calc(#{(($font-size-base * $line-height-base - $custom-control-indicator-size) / 2)} + #{$custom-control-indicator-border-width * 2})

I'm far from a math guy so if so if someone could assist me in these issues that would be much appreciated.

Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

LGTM, more control over sizing is something we can look into later

@mdo mdo mentioned this pull request Dec 12, 2018
@XhmikosR XhmikosR merged commit 180a06e into twbs:v4-dev Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.