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.

Sign up for GitHub

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

Moore/defaulting date range picker to last date #572

Merged
merged 7 commits into from
May 9, 2017
Merged

Moore/defaulting date range picker to last date #572

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
128f3ee
adding watch script for running locally
May 8, 2017
e462224
using selected end date as display if available
May 8, 2017
bf21de0
removing index from key
May 8, 2017
8eb2539
new lines and removing unused declared variable
May 8, 2017
894eb04
using selected end date for current date if it is present
May 8, 2017
f4d5ead
removing npm-watch
May 9, 2017
23c06d5
removing unused watch script and linked commands
May 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions src/components/DateRangePicker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,18 @@ DefaultRanges.propTypes = {
})
};

const MonthTable = ({ activeSelectDate, currentDate, getDateRangePosition, handleDateHover, handleDateSelect, isInActiveRange, minimumDate, selectedEndDate, selectedStartDate, styles }) => {
const MonthTable = ({
activeSelectDate,
currentDate,
getDateRangePosition,
handleDateHover,
handleDateSelect,
isInActiveRange,
minimumDate,
selectedEndDate,
selectedStartDate,
styles
}) => {
const days = [];
let startDate = moment.unix(currentDate).startOf('month').startOf('week');
const endDate = moment.unix(currentDate).endOf('month').endOf('week');
Expand Down Expand Up @@ -164,14 +175,17 @@ class DateRangePicker extends React.Component {
};

state = {
currentDate: this.props.selectedStartDate || moment().unix(),
currentDate: this.props.selectedEndDate || moment().unix(),
showCalendar: false
};

componentWillReceiveProps (newProps) {
if (newProps.selectedStartDate && newProps.selectedStartDate !== this.props.selectedStartDate) {
const isUpdatedSelectedEndDate = newProps.selectedEndDate && newProps.selectedEndDate !== this.props.selectedEndDate;
const isUpdatedSelectedStartDate = newProps.selectedStartDate && newProps.selectedStartDate !== this.props.selectedStartDate;

if (isUpdatedSelectedEndDate || isUpdatedSelectedStartDate) {
this.setState({
currentDate: newProps.selectedStartDate
currentDate: newProps.selectedEndDate ? newProps.selectedEndDate : newProps.selectedStartDate
});
}
}
Expand Down Expand Up @@ -355,7 +369,7 @@ class DateRangePicker extends React.Component {
type='caret-left'
/>
<div>
{moment(this.state.currentDate, 'X').format('MMMM YYYY')}
{moment(this.props.selectedEndDate ? this.props.selectedEndDate : this.state.currentDate, 'X').format('MMMM YYYY')}
</div>
<Icon
elementProps={{
Expand All @@ -367,13 +381,19 @@ class DateRangePicker extends React.Component {
/>
</div>
<div style={styles.calendarWeek}>
{['S', 'M', 'T', 'W', 'T', 'F', 'S'].map((day, i) => {
return (
<div key={day + i} style={styles.calendarWeekDay}>
{day}
</div>
);
})}
{[{ label: 'S', value: 'Sunday' },
{ label: 'M', value: 'Monday' },
{ label: 'T', value: 'Tuesday' },
{ label: 'W', value: 'Wednesday' },
{ label: 'T', value: 'Thursday' },
{ label: 'F', value: 'Friday' },
{ label: 'S', value: 'Saturday' }].map((day) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering why you decided to create objects here - what is the value key giving you - from what i see you're using it as the key prop, which seems like overkill?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is extremley important to give React keys to elements that are both consistent and unique.

Previously the solution was supplying keys that were unique but not consistent because it relied on the iteration index to create the key: key={day + i}.

In this case this is not currently causing problems, but it is one of my pet peeves and I wanted to fix it. I couldn't use the day value by itself because then it would be consistent but not unique.

So I created an array of objects with a label and a value. The value for each object is both consistent and unique, so it meets the constraints necessary to be used for a key by React to create iterable elements.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More background on the importance of unique and consistent keys: facebook/react#1342 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i'm cool with it. what isn't consistent about relying on the index? .map does honor the order of the original array.

Copy link
Collaborator Author

@paniclater paniclater May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If the array element changes, or the order changes then it will be a problem. Unlikely here because of the nature of days of the week and the way we're using the labels.

...... but what if we want to introduce dynamic localization to the app, and allow the user to switch between French and English.

Initial render keys would be "S0", "M1", "T2", "W3", "T4", "F5", "S6".
Next render keys would be "D0", "L1", "M2", "M3", "J4", "V5", "S6" <-- one consistent key, 6 inconsistent keys.

Now React is confused, it thinks that it has 6 new elements and one consistent one. In this case the Samedi and Saturday both start with the same initial so perhaps we don't recognize that there's a bug.

Then we add the option to have a longer display for the day of the week, now when we toggle back and forth between languages inconsistent bugs will show up, maybe only 5 elements show up sometimes, maybe "Sat" shows up with the English still in place for the French Saturday, maybe a combination of these behaviors because React will do a good job most of the time, but mess up part of the time since it can't guarantee consistent behavior with inconsistent or duplicate keys. These kinds of bugs could drive a person bonkers trying to tie down.

And this is in a simple example here where there is almost no dynamism. My real concern is people see this pattern and then copy it other places not realizing the potential harm.

return (
<div key={day.value} style={styles.calendarWeekDay}>
{day.label}
</div>
);
})}
</div>
<MonthTable
activeSelectDate={this.state.activeSelectDate}
Expand Down