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.

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

fix(time-picker): remove pre-populate value #995

Merged
merged 12 commits into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
93 changes: 93 additions & 0 deletions packages/elements/src/time-picker/__test__/time-picker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,45 @@ describe('time-picker/TimePicker', function () {
await elementUpdated(el);
expect(el.seconds).to.equal(null);
});

it('should not pre-populate other segments value when hours value changes', async function () {
const el = await fixture('<ef-time-picker></ef-time-picker>');
const hoursInput = el.hoursInput;
hoursInput.value = '12';
hoursInput.dispatchEvent(
new CustomEvent('focused-changed', { bubbles: true, detail: { value: false } })
);
await elementUpdated(el);
expect(el.hours).to.equal(12);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(null);
});

it('should not pre-populate other segments value when minutes value change', async function () {
const el = await fixture('<ef-time-picker></ef-time-picker>');
const minutesInput = el.minutesInput;
minutesInput.value = '30';
minutesInput.dispatchEvent(
new CustomEvent('focused-changed', { bubbles: true, detail: { value: false } })
);
await elementUpdated(el);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(30);
expect(el.seconds).to.equal(null);
});

it('should not pre-populate other segments value when seconds value change', async function () {
const el = await fixture('<ef-time-picker show-seconds></ef-time-picker>');
const secondsInput = el.secondsInput;
secondsInput.value = '45';
secondsInput.dispatchEvent(
new CustomEvent('focused-changed', { bubbles: true, detail: { value: false } })
);
await elementUpdated(el);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(45);
});
});

describe('Modes', function () {
Expand Down Expand Up @@ -561,5 +600,59 @@ describe('time-picker/TimePicker', function () {
expect(el.minutes).to.equal(0);
expect(el.hours).to.equal(0);
});

it('Cycling through seconds should not pre-populate other segments value', async function () {
el = await fixture('<ef-time-picker show-seconds></ef-time-picker>');
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(null);
createKeyboardEvent(el.secondsInput, InputKey.arrowDown);
await elementUpdated(el);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(0);

createKeyboardEvent(el.secondsInput, InputKey.arrowDown);
await elementUpdated(el);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(59);
});

it('Cycling through minutes should not pre-populate other segments value', async function () {
el = await fixture(timePickerDefaults);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(null);
createKeyboardEvent(el.minutesInput, InputKey.arrowDown);
await elementUpdated(el);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(0);
expect(el.seconds).to.equal(null);

createKeyboardEvent(el.minutesInput, InputKey.arrowDown);
await elementUpdated(el);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(59);
expect(el.seconds).to.equal(null);
});

it('Cycling through hours should not pre-populate other segments value', async function () {
el = await fixture(timePickerDefaults);
expect(el.hours).to.equal(null);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(null);
createKeyboardEvent(el.hoursInput, InputKey.arrowDown);
await elementUpdated(el);
expect(el.hours).to.equal(0);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(null);

createKeyboardEvent(el.hoursInput, InputKey.arrowDown);
await elementUpdated(el);
expect(el.hours).to.equal(23);
expect(el.minutes).to.equal(null);
expect(el.seconds).to.equal(null);
});
});
});
68 changes: 37 additions & 31 deletions packages/elements/src/time-picker/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
MILLISECONDS_IN_HOUR,
MILLISECONDS_IN_MINUTE,
MILLISECONDS_IN_SECOND,
MINUTES_IN_HOUR,
SECONDS_IN_MINUTE,
TimeFormat,
addOffset,
format,
Expand Down Expand Up @@ -49,6 +51,21 @@ const MAX_SECONDS = 59;
const HOURS_IN_DAY = 24;
const HOURS_OF_NOON = 12;

const SegmentMap = {
[Segment.HOURS]: {
milliseconds: MILLISECONDS_IN_HOUR,
cycle: HOURS_IN_DAY
},
[Segment.MINUTES]: {
milliseconds: MILLISECONDS_IN_MINUTE,
cycle: MINUTES_IN_HOUR
},
[Segment.SECONDS]: {
milliseconds: MILLISECONDS_IN_SECOND,
cycle: SECONDS_IN_MINUTE
}
};

const Placeholder = {
HOURS: '--',
MINUTES: '--',
Expand Down Expand Up @@ -230,7 +247,7 @@ export class TimePicker extends ControlElement {
}
}
public override get value(): string {
if (this.hours === null || this.minutes === null || (this.isShowSeconds && this.seconds === null)) {
if (!this.isCompleteValue) {
return '';
}
return this.currentTimeString;
Expand Down Expand Up @@ -317,6 +334,13 @@ export class TimePicker extends ControlElement {
return this.showSeconds || this.valueWithSeconds;
}

/**
* True if time value is complete, that is having all the required time segment
*/
private get isCompleteValue(): boolean {
Sakchai-Refinitiv marked this conversation as resolved.
Show resolved Hide resolved
return !(this.hours === null || this.minutes === null || (this.isShowSeconds && this.seconds === null));
}

/**
* Get hours taking into account AM/PM placeholder
*/
Expand Down Expand Up @@ -464,20 +488,6 @@ export class TimePicker extends ControlElement {
// no default
}

// Pre-populate empty segments
if (value !== null) {
if (segment === Segment.HOURS && this.minutes === null) {
this.minutes = 0;
}
if (
this.isShowSeconds &&
this.seconds === null &&
(segment === Segment.HOURS || segment === Segment.MINUTES)
) {
this.seconds = 0;
}
}

// verify value again, as time segment validation
// might fail in setter and previous value returned
if (oldValue !== this.value) {
Expand Down Expand Up @@ -722,28 +732,24 @@ export class TimePicker extends ControlElement {
/**
* Changes a time segment value by a specified amount.
* Also updates parent values when rolling through cycles.
* Incomplete value will update only segment without pre-populate value.
* @param amount Amount to change by
* @param segment Segment id
* @returns {void}
*/
private changeValueBy(amount: number, segment: Segment): void {
let offset = 0;
switch (segment) {
case Segment.HOURS:
offset = this.hours === null ? 0 : amount * MILLISECONDS_IN_HOUR;
break;
case Segment.MINUTES:
offset = this.minutes === null ? 0 : amount * MILLISECONDS_IN_MINUTE;
break;
case Segment.SECONDS:
offset = this.seconds === null ? 0 : amount * MILLISECONDS_IN_SECOND;
break;
// no default
const segmentValue = this[segment];
const { milliseconds, cycle } = SegmentMap[segment];

if (this.isCompleteValue) {
Sakchai-Refinitiv marked this conversation as resolved.
Show resolved Hide resolved
const offset = segmentValue === null ? 0 : amount * milliseconds;
const value = addOffset(this.currentTimeString, offset);
this.setValueAndNotify(value);
this.selectedSegment = segment;
} else {
// a segment cycle is added to support wrapping of amount with negative value
this[segment] = segmentValue === null ? 0 : (segmentValue + amount + cycle) % cycle;
wattachai-lseg marked this conversation as resolved.
Show resolved Hide resolved
}

const value = addOffset(this.currentTimeString, offset);
this.setValueAndNotify(value);
this.selectedSegment = segment;
}

/**
Expand Down
Loading