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 totally custom styles via 'bsStyle' #988

Closed
4 tasks
AlexKVal opened this issue Jul 14, 2015 · 16 comments
Closed
4 tasks

Allow totally custom styles via 'bsStyle' #988

AlexKVal opened this issue Jul 14, 2015 · 16 comments

Comments

@AlexKVal
Copy link
Member

For the code:

<ButtonGroup>
  <Button bsStyle='primary'>My button</Button>
  <Button bsStyle='myCustom'>My button</Button>
  <Button bsStyle='facebook'>My button</Button>
</ButtonGroup>

R-B generates markup: ('-undefined' suffix for every custom style name)
btn-undefined

Such will be possible: (for bsStyle='facebook' => 'facebook')
custom-styles-mappint

I will document the ability to use custom suffixes such as 'btn-myCustom' via:

styleMaps.addStyle('myCustom');

Now it is possible because of #496, but is not documented.

It will address:
this #537 (comment)
this #543 (comment)
and this #306 (comment)

you can't rely on bootstrap draft semantics and call it "rules".
It was built for user extension. Bootstrap info, warning, etc. were built more for demo purpose than for mindless usage in production code. That's BS developers opinion, not mine. 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 at all. Just allow users to pass whatever they want to. That simple.

And it will document this: #537 (comment)

  • Allow set custom styles without validation warnings through bsStyle property via styleMaps.addStyle(ComponentName, 'customStyleName') API
  • Document styleMaps.addStyle() API
@AlexKVal AlexKVal self-assigned this Jul 14, 2015
@AlexKVal
Copy link
Member Author

It's done, but I will push the PR after v0.24 is released.
Just don't want to load additional burned to our release engineers 😉

@AlexKVal
Copy link
Member Author

Where is the best place to put the Common documentation parts ? Such as:

Custom styles:

If you need to use your own custom style suffix instead of predefined ones (default, info, warning etc.) you can do it with like this:

import styleMaps from 'react-bootstrap/lib/styleMaps';
// or as
import { styleMaps } from 'react-bootstrap';
// and then add your custom style suffix via
styleMaps.addStyle('cool');

It will result in the following code becomes possible:

<Button bsStyle='cool' />
// it will generate this markup
<button class="btn btn-cool" />

And moreover: it will now validate your custom cool style together with the default ones.

I propose to create the new Common section exactly before Buttons section on Components page,
ore create a new Common (need a better name) page in the main nav bar.

And then add a link to this section to bsStyle type definition like this:
screen shot 2015-07-15 at 8 08 14 pm

What do you think ?

/cc @react-bootstrap/collaborators

@jquense
Copy link
Member

jquense commented Jul 15, 2015

I think its cool! I do think though that this probably needs to be solved along with the other issue on Style maps, i.e that you can add invalid styles to certain components.

if you add a modal style it should be a button style as well for instance. Same with how the 'link' style will validate correctly for modals but be invalid.

@AlexKVal
Copy link
Member Author

I am sorry. I did not get the point of what you said 😓
Could you provide, please, an example about what do you mean ?
What should else I add / remove / implement ?

@jquense
Copy link
Member

jquense commented Jul 15, 2015

I mean the second paragraph in #646 (comment), you can do <Button bsStyle='tabs'/> and the proptype validates but tabs is not a valid style for buttons.

@AlexKVal
Copy link
Member Author

This is just one of a dozen issues 😄 connected with it, and will be addressed with #646 or even after #646.
As you can see, they are not in a one place now:
#306 (comment)
It is on my todo list. I'm working on it already.

Though you did not answer the question: :)

What is the best way ?

  1. create the new Common section exactly before Buttons section on Components page
  2. create a new Common (need a better name) page in the main nav bar.

@jquense
Copy link
Member

jquense commented Jul 15, 2015

I think I would add just a section on "working with boostrap classes and variations" that details how to use bsStyle and then how to add custom ones.

I like the api you are proposing here, the only thing I think should happen is that custom styles should be scoped to specific components which will go with the other stuff you are working on :)

but something like: styleMaps.addStyle(Modal, 'cool')

@AlexKVal
Copy link
Member Author

Got it. Then I first refactor API:
styleMaps.addStyle('cool') => styleMaps.addStyle(Modal, 'cool')
and only then I will add documentation section about it.
And name it as "working with boostrap classes and variations".
Thank you for the hints.
🍒

@dozoisch
Copy link
Member

That looks super cool!

@AlexKVal
Copy link
Member Author

I PR the first part custom styles with no validation. #1034

The second one with validations through styleMaps.addStyle(Component, 'custom') api
can be added only in v0.25, because it will change the current api.
(though it is not documented, it is already in use #496)

@chellberg
Copy link

Mentioned this in #306 before noticing it was closed - it'd be nice if there was a way to conditionally set bsStyle without producing warnings.

e.g.

bsStyle={patientCurrentPhase === 'covered' ? 'info' : false

Unless I'm missing something, there's currently no falsey value that can be passed in to bsStyle property without generating warnings.

@jquense
Copy link
Member

jquense commented Aug 11, 2015

@chellberg setting it to undefined will probably work, if not then just set it to 'default'

@AlexKVal
Copy link
Member Author

Same here: #646 (comment)
I prefer to deal with that after #1096 and #962 are merged.

@AlexKVal AlexKVal removed their assignment Sep 23, 2015
@AlexKVal
Copy link
Member Author

#1257 is successor of this.

@chellberg
Copy link

@jquense forgot to reply, your suggestion did the trick! Thanks so much.

@raghuth
Copy link

raghuth commented Dec 1, 2017

good

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

5 participants