gpbl / react-day-picker

DayPicker is a customizable date picker component for React. Add date pickers, calendars, and date inputs to your web applications.
https://daypicker.dev
MIT License
6.08k stars 731 forks source link

Add support for CSS modules #73

Closed gpbl closed 7 years ago

gpbl commented 9 years ago

API proposal

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 commented 9 years ago

Yes, this would be great!

jfschwarz commented 8 years ago

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 commented 8 years ago

Thanks! :smile:

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 commented 8 years ago

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 commented 8 years ago

👍 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 commented 8 years ago

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

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?

philippe-git commented 8 years ago

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):

With this API and changes, I see two upsides:

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

209

kmamykin commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

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

wiesson commented 7 years ago

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

lgordey commented 7 years ago

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

gpbl commented 7 years ago

@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 commented 7 years ago

@gpbl Yeap, I already got it! Thanks!