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

Allow custom bsStyle #306

Closed
janmarek opened this issue Dec 10, 2014 · 31 comments
Closed

Allow custom bsStyle #306

janmarek opened this issue Dec 10, 2014 · 31 comments

Comments

@janmarek
Copy link

Now you can't add classes like btn-custom or alert-custom to your stylesheet and use them with react-bootstrap components. bsStyle property should be validated less strictly.

@trentgrover-wf
Copy link
Contributor

if you validate less strictly, you open yourself up to typo like problems (essentially you can't validate anything reliably at all). isn't it sufficient that you can provide btn-custom and such as optional css classes directly via className?

@janmarek
Copy link
Author

Unfortunately no, there would be className "btn btn-default btn-custom" instead of "btn btn-custom".

@trentgrover-wf
Copy link
Contributor

you can eliminate that by specifying bsStyle of empty string bsStyle=""

@janmarek
Copy link
Author

Ok, thanks.

<a className="btn btn-custom">click</a>
<Button bsStyle="" className="btn-custom">click</Button>

But it's probably nicer not to use react-bootstrap in this situation...

@ivan-kleshnin
Copy link

ivan-kleshnin commented Jan 22, 2015

@trentgrover-wf you can't rely on bootstrap draft semantics and call it "rules".
It was built for user extension. Bootstrap info, warning, etc. are there for demo purposes. End users may and should create their own sets of statuses etc. The current way of handling this is very uncomfortable, I 100% agree here with @janmarek. You don't need to validate class names against predefined sets.

@mtscout6
Copy link
Member

I can see the value in doing some kind of validation against a known set of styles you support, yet I can also see the value in allowing for custom styles. I think you can add your own custom styles and still keep the existing validation by doing:

var constants = require('react-boostrap/constants');
constants.STYLES.custom = 'custom';

The name constants is deceiving here since nothing has been done to ensure that those values have been modified. If you did this as one of your initial tasks prior to rendering anything, then you'd have the custom options you want with the validation to support it.

@mathieumg
Copy link
Member

Until the day these are enforced to be constant, or the filename changes.

@mtscout6
Copy link
Member

We can cross that bridge when that day comes 😄. I'm not planning to change that anytime soon.

@ivan-kleshnin
Copy link

@mtscout6 thank your for this trick, it may help.

@mtscout6
Copy link
Member

No problem!

@mtscout6
Copy link
Member

@janmarek Does the above suggestion help? Do we need to keep this issue open for any reason?

@janmarek
Copy link
Author

OK, I'm closing it.

@nemosupremo
Copy link

If you set bsStyle="", you get a ton of console errors where

Warning: Invalid prop `bsStyle` of value `` supplied to `Button`, expected one of ["default","primary","success","info","warning","danger","link","inline","tabs","pills"]. Check the render method of `DropdownButton`.

Unfortunately, if you aren't using CommonJS/AMD, then constants isn't exposed either. If bsStyle="" isn't a valid property bsStyle={false} or something like that should be enabled to prevent this validation. For us, we use a modified version of the bootstrap css (which I think is pretty common) and having designers provide a JS file listing out all the btn classes (or having developers hunt down all the btn classes in css) they used in a design seems like an unnecessary build step.

@ivan-kleshnin
Copy link

ivan-kleshnin commented Mar 5, 2015

@nemothekid, agree. Everyone modifies bootstrap for production. Nearly everyone adds custom class names for BS. I still hold my alredy expressed opionion that react-bootstrap devs just created a problem out of thin air with this. I don't care about class name validation at all (the cost of error is very low).

@mtscout6
Copy link
Member

mtscout6 commented Mar 5, 2015

@nemothekid I'd take a pull request to fix the problem with the constants not being available when using requirejs.

@trevorr
Copy link

trevorr commented Mar 12, 2015

@mtscout6's workaround for webpack (missing t typo fixed and lib added):

var constants = require('react-bootstrap/lib/constants');
constants.STYLES.custom = 'custom';

It's not a perfect workaround because it has to be included before anything else includes react-bootstrap, since BootstrapMixin copies the keys when loaded.

@trevorr
Copy link

trevorr commented Apr 4, 2015

Did v0.20.0 break the workaround of modifying constants? It's not working for me anymore.

@mtscout6
Copy link
Member

mtscout6 commented Apr 4, 2015

Thanks @trevorr I didn't realize that I missed that when we transitioned to using ES6 source. I just published a fix in version 0.20.1.

@AlexKVal
Copy link
Member

Maybe it would be appropriate to implement some kind of configuration option for disabling
all style checking at all for those who needs it ?
Maybe as temporary solution ?

@mtscout6
Copy link
Member

I don't think that's entirely necessary. All that happens with prop validation is console warnings. No errors, and #496 really did provide a nice means to include supported values by downstream users.

@AlexKVal
Copy link
Member

It seems that hardcoded "bottlenecks" exists only in three components now.
<Input />

propTypes: {
  bsStyle: React.PropTypes.oneOf(['success', 'warning', 'error'])
...

and
<FormGroup /> bsStyle: React.PropTypes.oneOf(['success', 'warning', 'error']),
<ListGroupItem /> bsStyle: React.PropTypes.oneOf(['danger', 'info', 'success', 'warning'])

Is there any value to implement something like #496 for these three components ?
I don't have enough expertise in react-bootstrap for this to answer.

@mtscout6
Copy link
Member

What's difficult about this is that bootstrap does not have the same set of styles for each component. For example the Button css styles support more types than Input of type text. It would be worth taking an inventory of each of the sets that bootstrap uses and make a decision with some better knowledge. What I think has happened is that we have been starting to see that disparity, and haven't figured out a decent solution for it.

@mtscout6
Copy link
Member

The other problem is that some folks are still able to write their own css which may add more available styles. So, for those components these users want to allow those added styles without console warnings. Having the global pool of options through the old constants and now styleMaps though is sub-optimal since it would not catch usage of styles that may not exist for a particular component. This needs more thought....

@chellberg
Copy link

+1 @nemothekid, would be great if bsStyle={false} suppressed the warnings. I'm trying to conditionally apply bsStyle and there doesn't appear to be a falsey option that doesn't generate warnings.

(thanks for the great library, guys)

@bbtfr
Copy link

bbtfr commented Nov 27, 2015

Seems we don't have react-bootstrap/lib/constants anymore?
+1 for bsStyle={false}
By the way, I found that it's possible to create a custom bsStyle for Button by changing styleMaps, but the bsStyle for Nav is hard-coded.

@taion
Copy link
Member

taion commented Nov 27, 2015

This got fully replaced now. We're not dropping validation anyway.

@tonyspiro
Copy link

Running into this also. Please update.

@tonyspiro
Copy link

Ok, looks like bsStyle={ null } suppresses the errors.

@Dragonqos
Copy link

tonyspiro - thanks, bsStyle={null} works great

@bbtfr
Copy link

bbtfr commented Dec 9, 2015

@tonyspiro 👍 thanks!

@thinkshihang
Copy link

@tonyspiro Thanks. bsStyle={null} +1

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

No branches or pull requests