Skip to content

Commit

Permalink
feat(datetime): default to now or max. Fixes #9846
Browse files Browse the repository at this point in the history
  • Loading branch information
mlynch committed Sep 29, 2017
1 parent f1a676e commit 559f4d3
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 2 deletions.
17 changes: 17 additions & 0 deletions src/components/datetime/datetime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,7 @@ export class DateTime extends BaseInput<DateTimeData> implements AfterContentIni
if (this.isFocus() || this._disabled) {
return;
}

console.debug('datetime, open picker');

// the user may have assigned some options specifically for the alert
Expand Down Expand Up @@ -548,6 +549,11 @@ export class DateTime extends BaseInput<DateTimeData> implements AfterContentIni
* @hidden
*/
generate() {
// If the date doesn't have a value yet, set it to now or the max value
if (Object.keys(this._value).length === 0) {
this._value = this.getDefaultValue();
}

const picker = this._picker;
// if a picker format wasn't provided, then fallback
// to use the display format
Expand Down Expand Up @@ -838,6 +844,17 @@ export class DateTime extends BaseInput<DateTimeData> implements AfterContentIni
}
}

/**
* Get the default value to show when none is provided,
* and within the bounds of the max value
* @private
*/
getDefaultValue() {
if (this.max) {
return parseDate(this.max);
}
return parseDate((new Date).toISOString());
}
}

/**
Expand Down
24 changes: 22 additions & 2 deletions src/components/datetime/test/datetime.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { DateTime } from '../datetime';
import { Form } from '../../../util/form';
import { Picker } from '../../picker/picker';
import { PickerController } from '../../picker/picker-controller';
import * as datetime from '../../../util/datetime-util';
import * as datetimeUtil from '../../../util/datetime-util';
import { mockApp, mockConfig, mockElementRef, mockRenderer } from '../../../util/mock-providers';


Expand All @@ -14,6 +14,26 @@ describe('DateTime', () => {

describe('validate', () => {

fit('should default to now if no initial value or bounds supplied', () => {
const now = datetimeUtil.parseDate(new Date().toISOString());
datetime.generate();
const val = datetime.getValue();
expect(val.year).toEqual(now.year);
expect(val.month).toEqual(now.month);
expect(val.day).toEqual(now.day);
expect(val.hour).toEqual(now.hour);
expect(val.minute).toEqual(now.minute);
});

fit('should default to max if no initial value supplied but max specified', () => {
datetime.max = '1987-10-19';
datetime.generate();
const val = datetime.getValue();
expect(val.year).toEqual(1987);
expect(val.month).toEqual(10);
expect(val.day).toEqual(19);
});

it('should restrict January 1-14, 2000 from selection, then allow it, and restrict December 15-31, 2001', () => {
datetime.max = '2001-12-15';
datetime.min = '2000-01-15';
Expand Down Expand Up @@ -683,7 +703,7 @@ describe('DateTime', () => {
console.warn = function(){};

// pt-br
var customLocale: datetime.LocaleData = {
var customLocale: datetimeUtil.LocaleData = {
dayNames: [
'domingo',
'segunda-feira',
Expand Down

9 comments on commit 559f4d3

@Simpler1
Copy link

@Simpler1 Simpler1 commented on 559f4d3 Sep 29, 2017

Choose a reason for hiding this comment

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

Hi Max,
Having a max value defined should only influence the default value if the max value is in the past. The default value should be now if the max value is in the future.

Thanks

@mlynch
Copy link
Contributor Author

@mlynch mlynch commented on 559f4d3 Sep 29, 2017

Choose a reason for hiding this comment

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

@Simpler1 there's an issue, max should be the current if it's before current, but not if it's after. Fix incoming

@Simpler1
Copy link

Choose a reason for hiding this comment

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

Perfect. Thanks

@WhatsThatItsPat
Copy link

Choose a reason for hiding this comment

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

Max, please reread this issue because this fix doesn't address what I think most of us are looking for.

Note, to set the default value, you really should just use ngModel and set the value on model. That way it's actually reflected in your code.

We are specifically trying to avoid this. Just as you wouldn't default a name field to "John Doe" and expect users to change it, you shouldn't have to default to a date and expect them to change it.

Added a fix to datetime to default to the current date/time or the max value if provided.

I'm struggling to think of why this would be useful. How many scenarios are there where the max date would be the most likely / closest to what the user wants to enter? If anything, the current date should override min or max.

Which brings us back to what we really want; a pickerPlaceholder. We want to set a date for the picker to open to in order to minimize the amount of fiddling the user has to do with the picker dials.

@mlynch
Copy link
Contributor Author

@mlynch mlynch commented on 559f4d3 Sep 29, 2017

Choose a reason for hiding this comment

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

You're right, those changes add in a different requested feature to set the default to some sane value which should be the current time. Will make another set of changes to make it possible to set the default

@shyamal890
Copy link

@shyamal890 shyamal890 commented on 559f4d3 Sep 29, 2017

Choose a reason for hiding this comment

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

Agree with @PatrickMcD all we want is an new attribute like placeholder which can be set to any date that the developer wishes to. Almost all UI frameworks like Angular-UI, Bootstrap have a placeholder attribute for date/time picker in addition to ngModel or picker-value.

@mlynch
Copy link
Contributor Author

@mlynch mlynch commented on 559f4d3 Sep 29, 2017

Choose a reason for hiding this comment

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

@shyamal890 https://twitter.com/Shyamal890/status/913673840340869121

I took this to mean the picker should default to the current time when opened if nothing else was specified. Those commits add this feature which it seems we should have had anyways.

Next is to do a default value you can specify.

@shyamal890
Copy link

@shyamal890 shyamal890 commented on 559f4d3 Sep 29, 2017

Choose a reason for hiding this comment

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

Thanks a lot, I actually meant to convey the following (Point 1):

  1. Currently when a user opens date-picker the default is 1 Jan 2017 and current date is 29 Sept 2017, even if a user wants to change to tomorrow's date, it takes a lot of efforts. That is where an attribute like placeholder would help the developers. In which case whenever the date-picker is opened, placeholder attribute would ACTIVATE current date (ngModel shouldn't be modified) and the user wouldn't have to scroll a lot.

  2. If user opens up the date picker, and decides not to select any date, with current commit ngModel would be set to current date which again is a bad UX design. ngModel shouldn't be updated until the user clicks on Done button. (Assuming I am right in my understanding of what the latest commit aims to achieve)

@shyamal890
Copy link

Choose a reason for hiding this comment

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

I checked on Angular-UI site. Here's the attribute name and it's functionality:

initDate (Default: null) - The initial date view when no model value is specified.

I guess initDate is a much better name than placeholder as placeholder may seem limited to input control and not extend to datepicker. Moreover, the following part is the most important one: initial date view when no model value is specified

Please sign in to comment.