Supporting Duration for Datetime attributes #650
Conversation
There was a problem hiding this comment.
Hi Parul, thank you for submitting this!
I left a few comments on the code, since this is your first JS PR so even if we eventually go a different direction, I figured getting feedback on the JS would be useful.
However, I wanted to point out a bigger picture issue here:
It seems that most of the code here focuses on parsing a textual value, and there are no affordances (GUI) to actually specify the value for the end user of the app, which was the primary task of resolving that issue. I would even be fine with the value being stored with that cryptic P syntax, if the end user never had to read it or write it. The end user currently has to type in a duration string like "5 hours, 2 minutes, 3 seconds". Is typing out words (and possibly making typos) the most efficient way for them to specify a duration? For example, when the input is a date, there is a date picker (which is native to the browser). When the input is a time, there is a time picker (also native to the browser). What UI would be efficient and learnable for a duration picker?
| }, | ||
| defaultFormats: { | ||
| "duration": name => "[getduration(".concat(name,"\"officialDurationString\")]"), | ||
| "duration-abbreviation": name => "[getduration(".concat(name,"\"abbreviatedDurationString\")]"), | ||
| "duration-normal": name => "[getduration(".concat(name,"\"normalDurationString\")]"), |
There was a problem hiding this comment.
We never touch the files in version folders, only the ones directly in dist, which are built automatically by gulp.
| @@ -94,6 +94,20 @@ $.extend(_, { | |||
| }, {multiValued: true}), | |||
|
|
|||
| localTimezone: -(new Date()).getTimezoneOffset(), | |||
| getduration: $.extend(function (dur, format) { | |||
There was a problem hiding this comment.
It's unclear what the purpose of getduration() is, or how reusable it is. It seems to take a formatted duration string, and return a different formatted duration string. I would suggest decoupling it into a function that parses a duration string and returns a number, and then we already have a function that takes a number and converts it to a formatted duration string (duration()).
A few more minor things:
- Avoid abbreviated names like
dur - inconsistent naming, camelCase in the line above, lowercase concatenated in this one. In general, JS tends to use camelCase, and it's usually good to follow the naming conventions of the host language.
Object.assign()should work fine instead of$.extend()here, the main difference being that the former executes accessors and assigns the result, whereas the latter transfers the accessors. When there are no accessors,Object.assign()is preferable as it's native to the platform, and thus faster.
| @@ -94,6 +94,20 @@ $.extend(_, { | |||
| }, {multiValued: true}), | |||
|
|
|||
| localTimezone: -(new Date()).getTimezoneOffset(), | |||
| getduration: $.extend(function (dur, format) { | |||
| if(format === "officialDurationString") {durationRegEx = /P(?:([.,\d]+)D)?(?:T(?:([.,\d]+)H)?(?:([.,\d]+)M)?(?:([.,\d]+)S)?)?/;} | |||
There was a problem hiding this comment.
Please install ESLint + an ESLint plugin for your editor so you can get inline warnings when the coding style of the project is not followed.
| }, | ||
| defaultFormats: { | ||
| "duration": name => "[getduration(".concat(name,"\"officialDurationString\")]"), | ||
| "duration-abbreviation": name => "[getduration(".concat(name,"\"abbreviatedDurationString\")]"), | ||
| "duration-normal": name => "[getduration(".concat(name,"\"normalDurationString\")]"), |
There was a problem hiding this comment.
Please use template strings for concatenation, which are more readable than .concat() method calls, and don't require escaping of double quotes. See the other values in this object literal for example.
| @@ -488,8 +488,14 @@ _.register({ | |||
| "time": /^[H\d]{2}:[M\d]{2}/i, | |||
| "datetime-local": /^[Y\d]{4}-[M\d]{2}-[D\d]{2} [H\d]{2}:[Mi\d]{2}/i, | |||
| "date": /^[Y\d]{4}-[M\d]{2}-[D\d]{2}$/i, | |||
| "duration": /^P/, | |||
| "duration-normal": /(day[s]?|hour[s]?|minute[s]?|second[s]?)$/, | |||
There was a problem hiding this comment.
Ah, I see what you meant now. Yes, we'd need to add ms, weeks, months, years too.
One direction to explore: can we make this work for other languages beyond English? (look at the Intl object)
| if(format === "officialDurationString") {durationRegEx = /P(?:([.,\d]+)D)?(?:T(?:([.,\d]+)H)?(?:([.,\d]+)M)?(?:([.,\d]+)S)?)?/;} | ||
| if(format === "abbreviatedDurationString") {durationRegEx = /[\s]*(?:([.,\d]+)d)?[\s]*(?:([.,\d]+)h)?[\s]*(?:([.,\d]+)m)?[\s]*(?:([.,\d]+)s)?[\s]*/;} | ||
| if(format === "normalDurationString") {durationRegEx = /[\s]*(?:([.,\d]+)[ ]?day[s]?)?[\s]*(?:([.,\d]+)[ ]?hour[s]?)?[\s]*(?:([.,\d]+)[ ]?minute[s]?)?[\s]*(?:([.,\d]+)[ ]?second[s]?)?[\s]*/;} | ||
| var matches = dur.match(durationRegEx); |
There was a problem hiding this comment.
So this is an interesting violation of DRY. Interesting because it's less obvious than simply duplicated code, but there is still more than one sources of truth. The dur parameter already contains the information about what format the parameter is in. These formats are mutually exclusive, and thus, there is no ambiguity that requires such a hint. Providing the format as a second parameter is done for developer convenience, not user convenience. It's better for your code to determine the format automatically by trying to parse as the 1st format, and failing that the 2nd format and so on.
Revert "Updated duration to return an Array and follow code syntax of file" This reverts commit cf2c191.
To addresss Issue #47
For example: datetime can be set to "P5DT2H4S", "5 days 2 hours 4 seconds", or "5d 2h 4s"