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

Reduce specificity of default styles #101

Closed
WickyNilliams opened this issue Jun 6, 2016 · 8 comments
Closed

Reduce specificity of default styles #101

WickyNilliams opened this issue Jun 6, 2016 · 8 comments

Comments

@WickyNilliams
Copy link

The BEM classes added to each component are super useful for applying custom styling. However, because the default styles have a high specificity (e.g. .react-tabs [role=tab][aria-selected=true]:hover), the purpose of adopting BEM is defeated slightly.

I propose using low specificity BEM classes for the default styles, so that they may be overridden more easily

@HonzaMikula
Copy link

The best would be possible turn off style entirely.

@darkyen
Copy link

darkyen commented Jul 24, 2016

Just a suggestion: Let us pass our own style?

@rhys-vdw
Copy link

rhys-vdw commented Aug 9, 2016

Just a suggestion: Let us pass our own style?

I don't like this option because it still forces the project to depend on js-stylesheet.

The best would be possible turn off style entirely.

This is currently possible @HonzaMikula, as a global here.

Yes. It would be better if js-stylesheet were not a dependency, and the styles could optionally be imported like so:

// These no longer require the styles or `js-stylesheet`...
import { Tab, Tabs, TabList, TabPanel } from 'react-tabs';

// ...So we must import ourselves...
import jss from 'js-stylesheet';

// If we didn't import this here, it would not be bundled into the project at all.
import TabStyles from 'react-tabs/styles';

class MyTabs {
  componentDidMount() {
    // Bit of extra work - but that's the point.
    jss(TabStyles);
  }

  render() {
    return (
      <Tabs>
         // blah blah
      </Tabs>
    );
  }
}

Right now this project adds js-styles as a dependency, even though my project doesn't use your styles option!

An alternate implementation would move js-stylesheet to optionalDependencies, and instead of importing it at the top of Tabs.js, it could be moved into here like so:

  componentDidMount() {
    if (useDefaultStyles) {
      // Will fail if *optional* dependency is not included - this responsibility is on the client.
      require('js-stylesheet')(require('../helpers/styles.js')); // eslint-disable-line global-require
    }
  },

But this approach is less optimal because the /helpers/styles.js file will still be bundled by webpack (which detects and replaces require statements).

Also if you wanted to do this in a back compatible way, the main package could export Tabs as a higher order component and have react-tabs/unstyled as the functional part of the component. The default exported Tabs component would be something like this:

import jss from 'js-styles';
import styles from './helper/styles';
import UnstyledTabs from './unstyled/tabs';

export default class Tabs {
  componentDidMount() {
    jss(styles);
  }

  render() {
    return <UnstyledTabs {...this.props} />;
  }
}

Actually that last one is probably the best. Want a PR, @danez? If so, which one?

@lautarodragan
Copy link

lautarodragan commented Mar 7, 2017

It'd be awesome if we could pass a prop useDefaultTyles={false}to disable default styles instead of calling Tabs.setUseDefaultStyles(false) globally. It feels more intuitive.

I think that for this to be possible the default styles would need to be included as inline styles (style={props.useDefaultStyles && defaultStyles}), otherwise one Tabs element set to use default styles would pollute the entire CSS and impact all other Tabs elements in the web app.

I think you could also drop the dependency on jss this way, or at least pave the way.

@danez danez mentioned this issue Apr 13, 2017
Merged
@lautarodragan
Copy link

lautarodragan commented Apr 13, 2017

@danez how did you resolve this in the end? I'm searching for useDefaultStyles on the diff of #161 and can only find it in the old part of the diff, not the new. Also I see you deleted helpers/styles.js entirely.

@danez
Copy link
Collaborator

danez commented Apr 13, 2017

The idea is to not force anyone to use a specific css technology. So instead of having style injected by default with js-stylesheets, react-tabs does not do anything by default regarding css (besides adding classnames to the rendered elements). It is up to the user to wire things up. There is a new file style/react-tabs.css which can be for example imported when using webpack css-loader

import 'react-tabs/style/react-tabs.css';

Or it can be simply copied.
I can maybe also add a sass/less variant that can be imported in someones own sass/less files.

It is a little bit more work to initially get the default style going, but a lot of people wanted to disable the default style (with js-stylesheet) anyway.

Does that sound reasonable? What do you think?

@WickyNilliams
Copy link
Author

Sounds like the right solution to me 👍

@lautarodragan
Copy link

Yep, sounds perfect to me!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants