arnthor3 / react-bootstrap-toggle

Bootstrap-toggle without the JQuery dependencies
Other
54 stars 16 forks source link

Discrepancies with CSS button sizes classes #5

Closed dule closed 7 years ago

dule commented 7 years ago

The component appears to use the classes btn-xs, btn-sm, btn-lg, which I think are the bootstrap3 classes, but the docs and the included bootstrap2-toggle css file references bootstrap2, which use the old classes (btn-large, btn-small, btn-mini). Is there a good way to still use the bootstrap2 classes?

arnthor3 commented 7 years ago

You are 100% right, could we add the old classes into the mix? If you have an idea of how to handle it then awesome. Maybe add an prop "useBootstrapTwoClasses" or something like that?

dule commented 7 years ago

Yea something like that would be great (an alternate naming suggestion for the prop could be bootstrapVersion: 2).

There's certainly a number of ways to address this. For example, a className prop, such that one can pass btn-mini into the component and have it use it as a class. Or instead of doing the mapping from mini to xs, let the user do the appropriate size selection (e.g., if a person is using bootstrap3, they would use xs, sm, lg for their sizes, but if someone is using bootstrap2, they'd use mini, small, large -- basically the same way the onstyle and offstyle are done). Since, as far as I can tell, the size prop is used solely for the size class name.

arnthor3 commented 7 years ago

Sounds good! What about exporting another component like this. import { BootstrapToggle2 } from 'react-bootstrap-toggle

That way there would be changes to the props and no one has to change anything?

dule commented 7 years ago

That is a good point; certainly consideration for backwards compatibility is important. I'd be happy with any of these solutions :)

arnthor3 commented 7 years ago

You can reopen if I messed something up, what I did was

import React from 'react';
import Toggle from './react-bootstrap-toggle';

export default class ReactBootstrap2Toggle extends Toggle {
  getSizeClass() {
    if (this.props.size === 'large') return 'btn-large';
    if (this.props.size === 'small') return 'btn-small';
    if (this.props.size === 'mini') return 'btn-mini';
    return '';
  }
}

A fairly cheap fix :)

dule commented 7 years ago

Great, thanks! Do you mind publishing 2.0.2? I can pull from npm and give this a try. Really appreciate the quick turnaround.

arnthor3 commented 7 years ago

Sure I am just going to add the test and then I will publish in 15 min or so.

dule commented 7 years ago

Thanks! Works great. One small thing, not sure if it's specific to bootstrap2 styling, but this span: https://github.com/arnthor3/react-bootstrap-toggle/blob/a8e0e71b31e9bd2f5ce8e4737e093fff14dc9e16/src/react-bootstrap-toggle.jsx#L139 seems to also require the sizeClass.

arnthor3 commented 7 years ago

Ok, so it should be easy to overwrite the render method in the new Bootstrap2Toggle class and add it there, right? I will try to fit this in tonight unless you want to do a pull request?

arnthor3 commented 7 years ago

Something like this?

export default class ReactBootstrap2Toggle extends Toggle {
  getSizeClass() {
    if (this.props.size === 'large') return 'btn-large';
    if (this.props.size === 'small') return 'btn-small';
    if (this.props.size === 'mini') return 'btn-mini';
    return '';
  }
  render() {
    const onstyle = `btn-${this.props.onstyle}`;
    const offstyle = `btn-${this.props.offstyle}`;
    const sizeClass = this.getSizeClass();
    const activeClass = `btn toggle ${sizeClass} ${onstyle}`;
    const inactiveClass = `btn toggle ${sizeClass} ${offstyle} off`;
    const onStyleClass = `btn toggle-on ${sizeClass} ${onstyle}`;
    const offStyleClass = `btn toggle-off ${sizeClass} ${offstyle}`;

    let style = {};
    if (this.props.width && this.props.height) {
      style = {
        width: this.props.width,
        height: this.props.height,
      };
    } else {
      style = {
        width: this.state.width,
        height: this.state.height,
      };
    }
    return (
      <div
        disabled={this.props.disabled}
        className={this.props.active ? activeClass : inactiveClass}
        onClick={this.onClick}
        style={style}
      >
        <div className="toggle-group">
          <span
            ref={(onLabel) => { this.on = onLabel; }}
            className={onStyleClass}
          >
            {this.props.on}
          </span>
          <span
            ref={(offLabel) => { this.off = offLabel; }}
            className={offStyleClass}
          >
            {this.props.off}
          </span>
          <span className={`toggle-handle btn btn-default ${sizeClass}`} />
        </div>
      </div>
    );
  }
}
dule commented 7 years ago

So I wanted to verify something before attempting to propose a change. Sorry, I should have done so before raising it. It doesn't appear to be bootstrap2 specific (seems to affect bs3 to some degree), and playing around with your example.jsx file the styling doesn't appear so bad there, but somehow worse in my project (probably some conflict in styling or an older bootstrap css file). It occurs more if you pass in a prop height, so if for example you have height: '20' and size: 'mini' the sizeClass on the handle span appears to scale it a bit better. I can easily adjust in my own local css, so it's probably something subjective and not requiring any changes. (Again, sorry)

arnthor3 commented 7 years ago

I looked it over and I noticed changes when trying out different size classes, I think it was the padding, not sure though. If this breaks anything it's just a one liner to get back :)

If you still get the proptype error I would love to get an update on that as well.

Thanks for the issues this was a lot of fun :)