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 CSS class to day name #441

Closed
wants to merge 2 commits into from

Conversation

montogeek
Copy link

Implements #440

Added a CSS class for days name.

@codecov-io
Copy link

Current coverage is 95.49%

Merging #441 into master will not affect coverage as of ad49014

@@            master    #441   diff @@
======================================
  Files            9       9       
  Stmts          222     222       
  Branches        45      45       
  Methods         91      91       
======================================
  Hit            212     212       
  Partial          2       2       
  Missed           8       8       

Review entire Coverage Diff as of ad49014

Powered by Codecov. Updated on successful CI builds.

@@ -159,7 +159,7 @@ var Calendar = React.createClass({
{this.renderCurrentMonth()}
{this.renderYearDropdown()}
{this.renderNextMonthButton()}
<div>
<div className="react-datepicker__day-name">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was envisioning this fix as changing the CSS class of the day headers, as in https://github.com/Hacker0x01/react-datepicker/blob/v0.25.0/src/calendar.jsx#L92

I think changing that class from react-datepicker_-day to react-datepicker__day-name makes much more sense

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't change it since it will lose react-datepicker__day styles, we have two alternatives:
Add react-datepicker__day-name class or add it to its container.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think it should be refactored to make it work with the __day-name class instead of __day. The things they will have in common are defined by variables that can be reused.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several ways:

  1. Change the class to __day-name and in the SCSS extend __day.
  2. Add the class __day-name without change the SCSS.

Since this project is using BEM, I would vote for the second option, is another day, the modifier is that is the name day.

Let me know what works best for you :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically -day isn't a modifier, and I don't think it should be because the day names and dates don't have much in common other than width and height. Go with #2

@@ -91,7 +91,7 @@ var Calendar = React.createClass({
return [0, 1, 2, 3, 4, 5, 6].map(offset => {
const day = startOfWeek.clone().add(offset, 'days')
return (
<div key={offset} className="react-datepicker__day">
<div key={offset} className="react-datepicker__day react-datepicker__day-name">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the __day class entirely

@rafeememon rafeememon mentioned this pull request Apr 8, 2016
@rafeememon
Copy link
Contributor

See #443

@rafeememon rafeememon closed this Apr 8, 2016
@montogeek montogeek deleted the css-class-for-day-name branch April 8, 2016 03:26
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

Successfully merging this pull request may close these issues.

None yet

3 participants