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

renderWeekDay property #172

Merged
merged 1 commit into from Jun 2, 2016

Conversation

3 participants
@stanislav-ermakov
Contributor

stanislav-ermakov commented May 31, 2016

Adds new renderWeekDay property which should contain a function that renders weekday headers

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 31, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c423057 on stanislav-ermakov-roi:weekday-renderer into 2f296cb on gpbl:master.

coveralls commented May 31, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c423057 on stanislav-ermakov-roi:weekday-renderer into 2f296cb on gpbl:master.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 31, 2016

Owner

Thanks @stanislav-ermakov-roi, nice idea! 👍

I wonder if it wouldn't be better to pass a prop with a ReactComponent class, instead of a function. Such component class would have the following props signature:

<Weekday weekday={ number } localeUtils={ object } locale={ string } />

then, when implementing the day picker, the developer would pass it via the new weekdayClass prop:

import DayPicker from 'react-day-picker';
import Weekday from './Weekday';

<DayPicker weekdayClass={ Weekday } />

In the DayPicker source code, we should write instead:

<div key={i} className="DayPicker-Weekday">
-   {renderWeekDay(i, locale, localeUtils)}
+   {React.createElement(this.props.weekdayClass, { weekday: i, locale, localeUtils })}
</div>

I understand you wanted to keep the similarity to renderDay prop :)

I was thinking to update that prop as well, e.g. passing a class: dayClass={ Day }. A similar idea was tracked in #89, using an element instead of a class.

What do you think?

Owner

gpbl commented May 31, 2016

Thanks @stanislav-ermakov-roi, nice idea! 👍

I wonder if it wouldn't be better to pass a prop with a ReactComponent class, instead of a function. Such component class would have the following props signature:

<Weekday weekday={ number } localeUtils={ object } locale={ string } />

then, when implementing the day picker, the developer would pass it via the new weekdayClass prop:

import DayPicker from 'react-day-picker';
import Weekday from './Weekday';

<DayPicker weekdayClass={ Weekday } />

In the DayPicker source code, we should write instead:

<div key={i} className="DayPicker-Weekday">
-   {renderWeekDay(i, locale, localeUtils)}
+   {React.createElement(this.props.weekdayClass, { weekday: i, locale, localeUtils })}
</div>

I understand you wanted to keep the similarity to renderDay prop :)

I was thinking to update that prop as well, e.g. passing a class: dayClass={ Day }. A similar idea was tracked in #89, using an element instead of a class.

What do you think?

@gpbl gpbl referenced this pull request May 31, 2016

Closed

New dayElement prop #89

@stanislav-ermakov

This comment has been minimized.

Show comment
Hide comment
@stanislav-ermakov

stanislav-ermakov May 31, 2016

Contributor

@gpbl, I think your idea is good and is inline with architecture of 2nd version of the component. I also need similar functionality for Navbar, since I need custom labels for next and previous buttons. So another pull request is coming very soon. I can update this one with approach you suggested, since I would like this to be merged very soon, if it's possible.

Regarding dayClass. Actually I would name this dayComponent, since class can be confused with className. Suggested idea could also be useful, but what if I just want to change cell content and not write all container props again? May be it's possible to update Day class to be able to extend it easily without the need to rewrite all the logic it contains already. But this should not be a problem if you keep renderDay prop.

Contributor

stanislav-ermakov commented May 31, 2016

@gpbl, I think your idea is good and is inline with architecture of 2nd version of the component. I also need similar functionality for Navbar, since I need custom labels for next and previous buttons. So another pull request is coming very soon. I can update this one with approach you suggested, since I would like this to be merged very soon, if it's possible.

Regarding dayClass. Actually I would name this dayComponent, since class can be confused with className. Suggested idea could also be useful, but what if I just want to change cell content and not write all container props again? May be it's possible to update Day class to be able to extend it easily without the need to rewrite all the logic it contains already. But this should not be a problem if you keep renderDay prop.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 31, 2016

Owner

@stanislav-ermakov-roi awesome! Then, better discuss the implementation details first, as I feel bad asking to review a PR :) Would you mind to join the gitter room?

Owner

gpbl commented May 31, 2016

@stanislav-ermakov-roi awesome! Then, better discuss the implementation details first, as I feel bad asking to review a PR :) Would you mind to join the gitter room?

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 31, 2016

Owner

Actually I would name this dayComponent, since class can be confused with className.

I see, yes, it should work... must be clear we are passing a component class (Day), not an element (<Day />).

but what if I just want to change cell content and not write all container props again

IMHO the component should not replace the container (e.g. that with DayPicker-day class), just the content. So is just about eventually renaming the prop.

I think as in my diff above, even weekdayComponent should just used for the content, The container with class name DayPicker-Weekday should stay there.

Owner

gpbl commented May 31, 2016

Actually I would name this dayComponent, since class can be confused with className.

I see, yes, it should work... must be clear we are passing a component class (Day), not an element (<Day />).

but what if I just want to change cell content and not write all container props again

IMHO the component should not replace the container (e.g. that with DayPicker-day class), just the content. So is just about eventually renaming the prop.

I think as in my diff above, even weekdayComponent should just used for the content, The container with class name DayPicker-Weekday should stay there.

@stanislav-ermakov

This comment has been minimized.

Show comment
Hide comment
@stanislav-ermakov

stanislav-ermakov May 31, 2016

Contributor

Ok. I see your point. Going to rework this pull request.

Contributor

stanislav-ermakov commented May 31, 2016

Ok. I see your point. Going to rework this pull request.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 31, 2016

Owner

@stanislav-ermakov-roi nevermind, while chatting comes out it's better the opposite about the container thing :)

Owner

gpbl commented May 31, 2016

@stanislav-ermakov-roi nevermind, while chatting comes out it's better the opposite about the container thing :)

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 31, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling cefbcb2 on stanislav-ermakov-roi:weekday-renderer into 2f296cb on gpbl:master.

coveralls commented May 31, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling cefbcb2 on stanislav-ermakov-roi:weekday-renderer into 2f296cb on gpbl:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 31, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 638ae77 on stanislav-ermakov-roi:weekday-renderer into 2f296cb on gpbl:master.

coveralls commented May 31, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 638ae77 on stanislav-ermakov-roi:weekday-renderer into 2f296cb on gpbl:master.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 1, 2016

Owner

Awesome thanks! Let me see why build fails then I'll merge :)

Owner

gpbl commented Jun 1, 2016

Awesome thanks! Let me see why build fails then I'll merge :)

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 2, 2016

Owner

The test was just bad written, my bad. Fixed!

Owner

gpbl commented Jun 2, 2016

The test was just bad written, my bad. Fixed!

@gpbl gpbl merged commit b07bae9 into gpbl:master Jun 2, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment