Moore/defaulting date range picker to last date #572
Conversation
package.json
Outdated
| @@ -57,6 +61,7 @@ | |||
| "eslint-plugin-react": "^5.0.0", | |||
| "live-server": "^0.8.1", | |||
| "node-libs-browser": "^0.5.2", | |||
| "npm-watch": "^0.1.9", | |||
There was a problem hiding this comment.
how does this watch functionality differ from what we already have in the webpack-dev-server?
There was a problem hiding this comment.
Oh sorry, I had intended to put this in a different pr. It is a way I've found to be able to have a pretty decent workflow when developing on applications that depend on npm libraries and the libraries themselves as well.
|
@lukeschunk ready for another review. |
| { label: 'W', value: 'Wednesday' }, | ||
| { label: 'T', value: 'Thursday' }, | ||
| { label: 'F', value: 'Friday' }, | ||
| { label: 'S', value: 'Saturday' }].map((day) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
More background on the importance of unique and consistent keys: facebook/react#1342 (comment)
There was a problem hiding this comment.
ok i'm cool with it. what isn't consistent about relying on the index? .map does honor the order of the original array.
There was a problem hiding this comment.
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.
|
|
This changes the DateRangePicker to use the selectedEndDate as the currentDate if possible, then fall back to the selectedStartDate if possible, finally falling back to the current day.
This makes it so that when the start and end date are selected the calendar shows the end of the range instead of the start.
No UI changes.
Issue: https://gitlab.mx.com/product/MoneyDesktop/issues/1591