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 navbarElement and weekdayElement props #179

Merged
merged 4 commits into from
Jun 10, 2016
Merged

Add navbarElement and weekdayElement props #179

merged 4 commits into from
Jun 10, 2016

Conversation

boatkorachal
Copy link
Contributor

from #177

@gpbl
Copy link
Owner

gpbl commented Jun 10, 2016

Hi @boatkorachal thanks for the help and congrats for your first PR 🎊

To maintain backward compatibility, we should not remove these *Component props. My idea was actually to add two new props. We could eventually deprecate them. Sorry for not being clear enough: I'll add some comments to your PR.

@gpbl
Copy link
Owner

gpbl commented Jun 10, 2016

(You should rebase your branch with master to fix these conflicts)

@@ -43,8 +43,8 @@ export default class DayPicker extends Component {
onCaptionClick: PropTypes.func,

renderDay: PropTypes.func,
weekdayComponent: PropTypes.func,
navbarComponent: PropTypes.func,
Copy link
Owner

@gpbl gpbl Jun 10, 2016

Choose a reason for hiding this comment

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

Keep those lat two lines. I'll add a PropType to deprecate them. Same for the other changes :)

@boatkorachal
Copy link
Contributor Author

ewww i just did the rebase and force push. hope that i haven't done anything wrong :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.732% when pulling 34bff97 on boatkorachal:navbar-and-weekday-element into 405d62a on gpbl:master.

@gpbl
Copy link
Owner

gpbl commented Jun 10, 2016

Yay thanks! Could you even test that the rendered component is actually the navbarElement? 🙏
https://coveralls.io/builds/6541993/source?filename=src%2FDayPicker.js#L357

@boatkorachal
Copy link
Contributor Author

Isn't it checked at proptypes? or should I import react-addons-test-utils and change to something like this

if ( TestUtils.isElement(navbarElement) ) {
  return React.cloneElement(navbarElement, props)
}

@gpbl
Copy link
Owner

gpbl commented Jun 10, 2016

The test coverage has been decreased, this is because we didn't include a test to check if passing a navbarElement did actually render the element. See https://coveralls.io/builds/6541993/source?filename=src%2FDayPicker.js#L357

Actually, even weekdayElement hasn't be checked: I wonder we are actually rendering it :)
I can't work on this now, so that's why I'm asking :)

@boatkorachal
Copy link
Contributor Author

I have just update tests. Is it ok? (I have to admit that this is the first time i write the tests too haha)

@boatkorachal
Copy link
Contributor Author

oh sorry i forgot to run lint. i'm gonna fix it now.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9878237 on boatkorachal:navbar-and-weekday-element into 405d62a on gpbl:master.

@gpbl gpbl merged commit e4fa65d into gpbl:master Jun 10, 2016
@gpbl
Copy link
Owner

gpbl commented Jun 10, 2016

Looks good! Thanks a lot!

@gpbl gpbl mentioned this pull request Jun 13, 2016
@gpbl gpbl changed the title navbarElement and weekdayElement instead of component Add navbarElement and weekdayElement props Jun 30, 2016
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.

3 participants