Add null/not null filters and fix the date picker#4
Conversation
dwat001
left a comment
dwat001
left a comment
There was a problem hiding this comment.
I don't have experience with grid.mvc so don't feel I should approve this change, but I have found some things worth commenting on.
Did the line endings of these files change? Github was doing a poor job of showing what was different.
This PR would of been easier to process if it was split into multiple commits.
There was a problem hiding this comment.
Could we either raygun this exception or check it is the expected exception and rethrow if it not the expected exception
There was a problem hiding this comment.
We can't Raygun from Grid.MVC as that'd add a dependency to it. I can delete this handler now the underlying format is fixed.
There was a problem hiding this comment.
A little bit of a shame to go from an unambiguous default to an ambiguous one.
10-11-12 could be a lot of dates 2010-11-12 is the 12th of november
There was a problem hiding this comment.
jQuery is special. yy-mm-dd is 2010-11-12. yyyy-mm-dd is 20102010-11-12.
There was a problem hiding this comment.
Do we want some validation that type is one of the date comparisons here? Or are we confident that onSelect will only fire when it is?
There was a problem hiding this comment.
.parseDate(datePickerOptions.format || "yy-mm-dd") ?
There was a problem hiding this comment.
can this block be removed from the try catch?
var date = null;
try{parse}catch(e){}
if(date) {...}
There was a problem hiding this comment.
If it worth doing formatting changes to a fork of a project? it will make taking updates from the upstream project harder. (I don't know how wide ranging our changes are).
There was a problem hiding this comment.
Mix of tabs and spaces, could we please keep this consistant within a file.
There was a problem hiding this comment.
Mix of space and tabs means code does not align.
There was a problem hiding this comment.
[Personal Preference] I prefer string[] over var as it makes it easier to determine the type of data.
There was a problem hiding this comment.
The use of var here means I don't know what types these are in code review. I love var as the left side of new statments, initilization of primitives, toString methods etc. ; but find it reduces the readability of most method calls
|
Didn't notice the whitespace changes, we default to tabs but grid.mvc is all spaces. I'll fix it up when I have time. |
Currently it cannot be changed after the initial value is set. This is because jQuery UI changed the event name and altered the dateParse code so that yyyy-mm-dd becomes 20102010-01-31 but yy-mm-dd is 2010-01-31
sitharus
force-pushed
the
fix-datepicker
branch
from
4983a5f to
60f8625
Compare
sitharus
force-pushed
the
fix-datepicker
branch
from
60f8625 to
56f0267
Compare
|
@dwat001 I've addressed your comments, it should be easier to digest now. I dumped it all up in a hurry before moving house. |
|
https://pushpay.atlassian.net/browse/PP-10280 needs this fix. |
3 participants
No description provided.