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

Add support for CSS modules #73

Closed
gpbl opened this issue Oct 13, 2015 · 18 comments
Closed

Add support for CSS modules #73

gpbl opened this issue Oct 13, 2015 · 18 comments
Milestone

Comments

@gpbl
Copy link
Owner

gpbl commented Oct 13, 2015

API proposal

  • a new prop classNames receiving an object containing the class names for the single elements, such as container, day, caption, ecc. Must be validated via PropTypes.
  • modifiers passed to the modifiers prop must return a string instead of a boolean. This string will be added to the day's cell class name. For CSS modules, it should be the imported class name.
    • if the returned value is falsy, no class name will be added to the day cell.

Note that this change would work also for any class names, e.g. people adopting other convention than BEM can specify their own class names.

Example using CSS Modules

import classNames from './daypicker.css';
import { todayClassName, weekendClassName } from './daypicker-modifiers.css';
import { isToday, isWeekend } from './myDateUtils';

const modifiers = {
  today: day => {
    if (isToday(day)) {
      return todayClassName;
    }
    return '';
  },
  weekend: day => {
    if (isWeekend(day)) {
      return weekendClassName;
    }
    return '';
  },
};

function MyComponent() {
  return (
    <div>
      <DayPicker classNames={classNames} modifiers={modifiers} />
    </div>
  );
}
@idolize
Copy link

idolize commented Nov 10, 2015

Yes, this would be great!

@gpbl gpbl modified the milestone: v2 Mar 2, 2016
@jfschwarz
Copy link

We are successfully using a fork of react-day-picker with inline styles: https://github.com/effektif/react-day-picker-substyled

The fork uses a little helper function to allow users to pass a nested object in the style prop. It also only assigns class names to the DOM nodes if the DayPicker has a className prop set. (Then all nested element class names are derived from that prop to avoid any global namespace pollution.)

If you like this approach, I'd be happy to contribute a PR.

Thanks for the most carefully crafted React datepicker implementation out there!

@gpbl
Copy link
Owner Author

gpbl commented Mar 22, 2016

Thanks! 😄

Substyle is interesting – however I'd prefer to keep the component without external dependencies.

Since I've never used CSS modules or inline style dynamically, I wonder if an API like this could work:

const dayPickerStyles = { ..
  container: styles.container,
  month: styles.month,
  navBar: styles.navBar
  /* ... etc */
}

function myComponent() {
 return <DayPicker classNames={ styles } modifiers={ modifiers } />;
}

What do you think?

@jfschwarz
Copy link

I do quite like this API. It's definitely a good solution for supporting css modules. I would probably rename the styles prop to classNames as that's more what it is.

It doesn't add support for inline styles, which is our main requirement at Signavio. However, I see that this might be out of the scope of this issue...

@philippe-git
Copy link

philippe-git commented May 4, 2016

👍 for that API and using the className prop to pass that config around, feels like a nice way to expose those components' class names without having access to the components themselves!

We're currently using CSS Modules for the project we're also using react-day-picker in, and so far having BEM-like namespacing felt sufficient (and using :global() to have those classes available globally), but CSS Modules support would be a nice improvement! Happy to help testing anything if needed!

@gpbl gpbl removed this from the v2 milestone May 16, 2016
@gpbl
Copy link
Owner Author

gpbl commented May 26, 2016

Now that v2 is out and the code is testable again :) I'd like to start implementing this feature.

I've changed the API proposal a bit, as the original idea that was complicating too much the component internals:

API proposal

  • a new prop classNames receiving an object containing the class names for the single elements, such as container, day, caption, ecc. Must be validated via PropTypes.
  • modifiers passed to the modifiers prop must return a string instead of a boolean. This string will be added to the day's cell class name. For CSS modules, it should be the imported class name.
    • if the returned value is falsy, no class name will be added to the day cell.

Note that this change would work also for any class names, e.g. people adopting other convention than BEM can specify their own class names.

Example using CSS Modules

import classNames from './daypicker.css';
import { todayClassName, weekendClassName } from './daypicker-modifiers.css';
import { isToday, isWeekend } from './myDateUtils';

const modifiers = {
  today: day => {
    if (isToday(day)) {
      return todayClassName;
    }
    return '';
  },
  weekend: day => {
    if (isWeekend(day)) {
      return weekendClassName;
    }
    return '';
  },
};

function MyComponent() {
  return (
    <div>
      <DayPicker classNames={classNames} modifiers={modifiers} />
    </div>
  );
}

If I've understood correctly how CSS modules work, this change should be enough for supporting them. Any feedback?

@gpbl gpbl changed the title Support for CSS modules Add support for CSS modules May 26, 2016
@gpbl gpbl modified the milestone: Next May 27, 2016
@philippe-git
Copy link

Hey! Thanks for the updated API proposal, it looks great and flexible!

I was thinking that for the purpose of supporting CSS Modules, we might be able to get away with something even simpler and just as flexible, and would love to get your thoughts 😄

With the current BEM-like approach, the flow with modifiers is to run the modifier functions, and attach a bunch of additional class names based on the returned value of those functions. The identifier used as the "modifier name" (as per the BEM lingo) in those additional class names is no other than the property name on the modifiers prop.

In other words:

<DayPicker modifiers={{ isSomething: () => true }} />

Will make all day cells have the DayPicker-Day--isSomething class; add another modifier that returns true for that day, and that's another class added to that day using the same format.

The main difference I see between this BEM-like approach and CSS Modules is that CSS Modules works with object properties that store class names, rather than class names directly.

What I'm getting at is that although with CSS Modules class names are resolved in a different way, it seems like we could also benefit from the determinism described above – and that'd make the API simpler 😄

With that in mind, I think the only change that could be needed for the API is the addition of a classNames prop as you suggested. This prop would store the CSS Module object, where each property name would map to a desired and pre-determined name. E.g. if using the isSomething modifier in the example above, a CSS Modules user would just need to create a stylesheet containing a DayPicker-Day--isSomething property, exactly like a BEM user would: .DayPicker-Day--isSomething { color: red }.

The only other changes necessary would be internally, where the day cell's className prop would be generated using two different methods, based on the presence of the classNames prop or not (denoting "CSS Modules mode" or not):

  • "BEM mode" (what's used currently):

    className += modifiers.map(modifier => ` ${className}--${modifier}`).join('');
    
  • "CSS Modules mode":

    className += modifiers.map(modifier => {
      const propName = `${className}--${modifier}`;
      const className = classNames[propName];
      return ` ${className}`;
    }).join('');
    

With this API and changes, I see two upsides:

  • All developers would be able to use the same stylesheets, without modifications
  • Modifiers could keep returning boolean values
  • Developers using CSS Modules would just have to pass their imported CSS Modules styles to the classNames prop, and be done with it

With that API, your previous example would look like:

import styles from './daypicker.css'; // Default + modifier styles
import { isToday, isWeekend } from './myDateUtils';

const modifiers = {
  today: day => isToday(day),
  weekend: day => isWeekend(day),
};

function MyComponent() {
  return (
    <div>
      <DayPicker classNames={styles} modifiers={modifiers} />
    </div>
  );
}

One downside I'm seeing with that way of handling CSS Modules is that it doesn't fit the CSS Modules spirit very well, in that we generally like to compose styles within stylesheets rather than composing multiple class names inside the same element (i.e. <div class="activeDay"/> .day {} .activeDay { composes: day; color: red; } instead of <div class="day day--active"/> .day {} .day--active { color: red; }), but with the flexibility the modifier system brings to days and the number of states they can simultaneously be in, nothing comes to mind that'd be more elegant 😄

That's just a quick thought I had so I may have overlooked some important stuff, but I'd love to know what you think!

P.S. Another downside is that this API wouldn't make room for "people adopting other convention than BEM" so that they can "specify their own class names" like you mentioned in your API proposal. I think another small and additional change on top of what I'm suggesting could make that possible though: if that feels like a valuable feature for other users, we could make the format of those generated identifiers configurable by exposing some sort of "template prop" or function. Since code is often clearer, maybe something like <DayPicker classIdentifierTemplate="$BASE-hey-$MOD" /> or <DayPicker classIdentifierFn={(baseClass, modifierName) => $(baseClass)-hey-$(modifierName)} /> which would be even more flexible, where both would generate class names like "DayPicker-Day-hey-isSomething" 😃

@gpbl
Copy link
Owner Author

gpbl commented May 29, 2016

Hi @pioul, thanks a lot for taking your time for reviewing it – really useful 👍

E.g. if using the isSomething modifier in the example above, a CSS Modules user would just need to create a stylesheet containing a DayPicker-Day--isSomething property, exactly like a BEM user would

Why should the exported CSS modules keep the BEM syntax, such as DayPicker-Day--isSomething, and not just isSomething? Is this because some best practices? Or just to keep backward compatibility?

className += modifiers.map(modifier => {
- const propName = `${className}--${modifier}`;
- const className = classNames[propName];
+ const className = classNames[modifier];
  return ` ${className}`;
}).join('');

Maybe I'm missing something as I'm not used to CSS Modules 😄

Another downside is that this API wouldn't make room for "people adopting other convention than BEM" so that they can "specify their own class names" like you mentioned in your API proposal.

No problem, it was just a side effect. I don't see big reasons for supporting this.

@philippe-git
Copy link

Hey @gpbl, that's a great question, and the points you've mentioned are spot on!

Since we'll be using a single stylesheet to style several elements, it feels good to continue using their names (e.g. "daypicker", "month", "navbar") in the class names. As far as conventions go for CSS Modules, if that was the only constraint, we could definitely use something simpler like dayIsSomething.

But like you've mentioned, since we'd like that approach to be compatible with non-CSS Modules users, using the exact same identifiers feels like the easiest way to go.

In that sense, what I suggest feels more like stuffing CSS Modules inside the current approach: it's functional, but not tailored for CSS Modules 😄 That'll allow CSS Modules users to start using react-day-picker the same way other users do, by simply copying (and tweaking if they'd like) style.css, and importing it inside their app – only they'll be using a named import and passing that style object through the classNames prop 😃

Behind the scenes, the day picker's styles will be local and exempt of any side-effects; allowing custom and arguably better classnames feels like a nice-to-have, considering the larger amount of work that'd be needed (i.e. like you've explored yourself, offering a way to map class names to elements)

Hope that makes sense!

@frandsaw
Copy link
Contributor

#209

@kmamykin
Copy link

kmamykin commented Dec 7, 2016

I think the approach taken in https://github.com/javivelasco/react-css-themr makes a lot of sense to me. Its compliant with CSS Modules and gives ability to add styles to component by explicitly passing classnames through properties, or by using a global theme(ThemeProvider). I have used it indirectly through http://react-toolbox.com/#/install and it works really well.

@chrisy-lili
Copy link

@gpbl Is Supporting CSS Modules and inline styles still in your future release plan? Like pioul mentioned, local styles protect day pickers from picking up unwanted styles in larger applications. I noticed there're a few forked versions that support it, but it'll be nice to see the original library provide the support to CSS Modules and inline styles too.

@gpbl gpbl modified the milestones: v4, v5 Feb 10, 2017
@gpbl gpbl closed this as completed in 028decb Feb 25, 2017
@gpbl gpbl reopened this Feb 25, 2017
@gpbl
Copy link
Owner Author

gpbl commented Feb 25, 2017

Hi people

support for CSS modules has been published under the @next tag. You can test it before I publish the next version:

npm install react-day-picker@next

This should close the issue, I'm new to CSS modules so I hope this is the right way to implement them :)

Thanks for your feedback!

@gpbl
Copy link
Owner Author

gpbl commented Mar 3, 2017

CSS Modules support has been added, published as v5.1.1.

http://react-day-picker.js.org/docs/styling.html

@gpbl gpbl closed this as completed Mar 3, 2017
@wiesson
Copy link

wiesson commented Jul 25, 2017

Just would like to let you know that I'm using this feature with https://github.com/cssinjs/jss and it works perfectly! 👍

@lgordey
Copy link

lgordey commented Jul 27, 2017

@wiesson hi man!
Can you give an example of working react-day-picker with jss?
Would be perfect 👍

@gpbl
Copy link
Owner Author

gpbl commented Jul 28, 2017

@lgordey Never tried jss myself, however looking to the JSS project's README, it should the same, i.e. using the classNames prop:

const {classes} = jss.createStyleSheet(styles).attach()

<DayPicker classNames={classes} />

Please refer to the classNames documentation.

@lgordey
Copy link

lgordey commented Jul 28, 2017

@gpbl Yeap, I already got it!
Thanks!

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

9 participants