Skip to content

Conversation

@chrismanciero
Copy link
Contributor

Description

Set the currentYear value to the selected date object.

fixes #357

@chrismanciero chrismanciero requested review from a team and jbadan February 5, 2019 17:58
this.setState({
currentDateDisplayed: date,
currentYear: date,
selectedDate: date,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it strange that we have 3 different state variables (currentDateDisplayed, currentYear, selectedDate) that are all being set to the same value? Seems like they should either be set to different values or the code that depends on them should just reference a single stored value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@greg-a-smith I agree, but I think it might be better to save that work for an overall refactoring effort instead of putting more time into this. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jbadan Agreed. That's probably a much larger effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree we should refactor this control. as a side not i did remove the currentYear property and updated the code and tests to work off the currentDateDisplayed property instead 😄

Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

I like this better! 🚢

@chrismanciero chrismanciero merged commit 4bb7ca9 into master Feb 7, 2019
@chrismanciero chrismanciero deleted the fix/Calendar-currentYear-357 branch February 7, 2019 18:03
greg-a-smith pushed a commit that referenced this pull request Mar 5, 2019
* fix: update currentYear on date selection

* removed currentYear prop
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.

Calendar Component: currentYear should be updated on date selection for consistentcy

4 participants