narqo / react-islands

react-islands Components Library
MIT License
120 stars 13 forks source link

Theme="islands" by default #56

Open vitkarpov opened 8 years ago

vitkarpov commented 8 years ago

Does it really need to specify the value of theme property manually? Seems the library doesn't support any other themes and implements only Yandex Islands style, so maybe theme should be defined as islands by default?

pasaran commented 8 years ago

You can use App to set default theme:

<App theme="islands">
    <Button>Islands</Button>
</App>

But actually I agree.

narqo commented 8 years ago

Does it really need to specify the value of theme property manually? Seems the library doesn't support any other themes and implements only Yandex Islands style.

Well, if we're talking about styles, the library doesn't have any own styles, as you might've noticed. Never the less, I think that in most cases, a theme should be set globally on the application level, as @pasaran has suggested. In this case, there is no need to have any predefined theme in components, don't you think so?

narqo commented 8 years ago

Just a small note about our App component: every component defines a theme as a prop that might be set through the context. So actually, you can do it in any of your own top-level components:

class Page extends Component {
  getChildContext() {
    return { theme: 'islands' };
  }

  static childContextTypes = {
    theme: React.PropTypes.string,
  }
}
vitkarpov commented 8 years ago

In this case, there is no need to have any predefined theme in components, don't you think so?

Seems it would be really convenient to have one. If I need to redefine it I always can set it manually using props or child context, but seems it's a kind of theory case.

narqo commented 8 years ago

Recently, I faced the case where default theme would be really harmful:

I'm in the process of rewriting a project that uses bootstrap heavily to react-islands. In order to do it gracefully, I'm changing separate parts of the page to these components, while leaving CSS classes and styles from bootstrap.

If Link and Button had a default theme it would overlap with the theme from bootstrap. The only solution for this problem, that I can think of would be a temporal "fake" theme to components.

Worth to say, this is only valid for simple components like Link and Button and in I'm strongly against of such usage in general. But I think an ability to not have a theme at all is a feature that shouldn't be rejected.