Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Bug 978547 - [Clock] JavaScript functions should not rely on presence an... #18342

Merged
merged 1 commit into from Apr 22, 2014
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions apps/clock/js/panels/alarm/alarm_list.js
Expand Up @@ -82,7 +82,6 @@ AlarmListPanel.prototype = {
var d = new Date();
d.setHours(alarm.hour);
d.setMinutes(alarm.minute);
var localeTime = Utils.getLocaleTime(d);

li.id = 'alarm-' + alarm.id;
li.dataset.id = alarm.id;
Expand All @@ -95,8 +94,7 @@ AlarmListPanel.prototype = {
link.classList.toggle('with-repeat', alarm.isRepeating());
link.dataset.id = alarm.id;

li.querySelector('.time-part').textContent = localeTime.time;
li.querySelector('.period').textContent = localeTime.ampm;
li.querySelector('.time').innerHTML = Utils.getLocalizedTimeHtml(d);
li.querySelector('.label').textContent = alarm.label || _('alarm');
li.querySelector('.repeat').textContent =
(alarm.isRepeating() ? alarm.summarizeDaysOfWeek() : '');
Expand Down
14 changes: 2 additions & 12 deletions apps/clock/js/panels/alarm/clock_view.js
Expand Up @@ -68,11 +68,6 @@ var ClockView = {
return (this.time = document.getElementById('clock-time'));
},

get hourState() {
delete this.hourState;
return (this.hourState = document.getElementById('clock-hour24-state'));
},

get dayDate() {
delete this.dayDate;
return (this.dayDate = document.getElementById('clock-day-date'));
Expand Down Expand Up @@ -153,10 +148,7 @@ var ClockView = {
opts = opts || {};

var d = new Date();
var time = Utils.getLocaleTime(d);
this.time.textContent = time.time;
this.hourState.textContent = time.ampm || '  '; // 2 non-break spaces

this.time.innerHTML = Utils.getLocalizedTimeHtml(d);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this A LOT. - abstract all the things!

this.timeouts.digital = setTimeout(
this.updateDigitalClock.bind(this), (60 - d.getSeconds()) * 1000
);
Expand All @@ -179,9 +171,7 @@ var ClockView = {
this.setTransform('hour', hour);

// Update aria label for analog view.
var time = Utils.getLocaleTime(now);
this.container.setAttribute(
'aria-label', time.t + (time.p ? ' ' : '') + time.p);
this.container.setAttribute('aria-label', Utils.getLocalizedTimeText(now));

// update again in one second
this.timeouts.analog = setTimeout(
Expand Down
5 changes: 1 addition & 4 deletions apps/clock/js/panels/alarm/list_item.html
Expand Up @@ -4,10 +4,7 @@
<span></span>
</label>
<a href="#alarm-edit-panel" class="alarm-item" data-id="">
<span class="time">
<span class="time-part"></span>
<span class="period"></span>
</span>
<span class="time"></span>
<span class="label"></span>
<span class="repeat"></span>
</a>
Expand Down
1 change: 0 additions & 1 deletion apps/clock/js/panels/alarm/panel.html
Expand Up @@ -13,7 +13,6 @@
<div id="digital-clock">
<div id="digital-clock-face">
<span id="clock-time"></span>
<span id="clock-hour24-state"></span>
</div>
</div>
<div id="clock-day-date"></div>
Expand Down
63 changes: 18 additions & 45 deletions apps/clock/js/panels/alarm_edit/main.js
Expand Up @@ -47,8 +47,12 @@ var AlarmEdit = function() {

this.buttons.time = new FormButton(this.selects.time, {
formatLabel: function(value) {
var time = Utils.parseTime(value);
return Utils.format.time(time.hour, time.minute);
var date = new Date();
// This split(':') is locale-independent per HTML5 <input type=time>
var splitValue = value.split(':');
date.setHours(splitValue[0]);
date.setMinutes(splitValue[1]);
return Utils.getLocalizedTimeText(date);
}.bind(this)
});
this.buttons.repeat = new FormButton(this.selects.repeat, {
Expand All @@ -69,6 +73,9 @@ var AlarmEdit = function() {
}
});

this.scrollList = this.element.querySelector('#edit-alarm');
this.sundayListItem = this.element.querySelector('#repeat-select-sunday');

// When the system pops up the ValueSelector, it inadvertently
// messes with the scrollTop of the current panel. This is a
// workaround for bug 981255 until the Edit panel becomes a new
Expand Down Expand Up @@ -104,47 +111,9 @@ var AlarmEdit = function() {

AlarmEdit.prototype = Object.create(Panel.prototype);

var selectors = {
scrollList: '#edit-alarm',
labelInput: 'input[name="alarm.label"]',
timeSelect: '#time-select',
timeMenu: '#time-menu',
alarmTitle: '#alarm-title',
repeatMenu: '#repeat-menu',
repeatSelect: '#repeat-select',
sundayListItem: '#repeat-select-sunday',
soundMenu: '#sound-menu',
soundSelect: '#sound-select',
snoozeMenu: '#snooze-menu',
snoozeSelect: '#snooze-select',
deleteButton: '#alarm-delete',
backButton: '#alarm-close',
doneButton: '#alarm-done'
};
Object.keys(selectors).forEach(function(attr) {
var selector = selectors[attr];
Object.defineProperty(AlarmEdit.prototype, attr, {
get: function() {
var element = this.element.querySelector(selector);
Object.defineProperty(this, attr, {
value: element
});
return element;
},
configurable: true
});
});

Utils.extend(AlarmEdit.prototype, {

alarm: null,
alarmRef: null,
timePicker: {
hour: null,
minute: null,
hour24State: null,
is12hFormat: false
},
ringtonePlayer: AudioManager.createAudioPlayer(),

handleNameInput: function(evt) {
Expand Down Expand Up @@ -249,15 +218,19 @@ Utils.extend(AlarmEdit.prototype, {
},

initTimeSelect: function aev_initTimeSelect() {
// The format of input type="time" should be in HH:MM
var opts = { meridian: false, padHours: true };
var time = Utils.format.time(this.alarm.hour, this.alarm.minute, opts);
this.buttons.time.value = time;
// HTML5 <input type=time> expects 24-hour HH:MM format.
var hour = parseInt(this.alarm.hour, 10);
var minute = parseInt(this.alarm.minute, 10);
this.selects.time.value = (hour < 10 ? '0' : '') + hour +
':' + (minute < 10 ? '0' : '') + minute;
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be good if we reused some pad() function and/or if utils had a method formatTime(hour, minute) - so we decouple the value parsing and DOM manipulation...

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we even do formatAlarmTime(alarm) to make it even simpler.

},

getTimeSelect: function aev_getTimeSelect() {
return Utils.parseTime(this.selects.time.value);
// HTML5 <input type=time> returns data in 24-hour HH:MM format.
var splitTime = this.selects.time.value.split(':');
return { hour: splitTime[0], minute: splitTime[1] };
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you decide to remove the Utils.parseTime? It looks like this logic would be useful in other places and old method was simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm.. I was just looking the reason why the getTimeSelect method exists and it's only called once and only to build an array with the values. So maybe just return the array directly to simplify things? that way no need for abstractions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree; in the interest of time, I'm going to push that forward to a future commit -- I'd like to get rid of many of these helper functions in the views which are only used once to return the value; perhaps getting rid of all of these at once.

},

initRepeatSelect: function aev_initRepeatSelect() {
this.buttons.repeat.value = this.alarm.repeat;
},
Expand Down
5 changes: 1 addition & 4 deletions apps/clock/js/ring_view.js
Expand Up @@ -113,9 +113,7 @@ RingView.prototype = {
this.ringDisplay.dataset.ringType = alert.type;

// Set the time to display.
var localeTime = Utils.getLocaleTime(alert.time);
this.time.textContent = localeTime.time;
this.hourState.textContent = localeTime.ampm;
this.time.innerHTML = Utils.getLocalizedTimeHtml(alert.time);
Copy link
Contributor

Choose a reason for hiding this comment

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

so much better 👍


if (alert.sound) {
this.ringtonePlayer.playRingtone(alert.sound);
Expand Down Expand Up @@ -228,7 +226,6 @@ RingView.prototype = {

Utils.extendWithDomGetters(RingView.prototype, {
time: '#ring-clock-time',
hourState: '#ring-clock-hour24-state',
ringLabel: '#ring-label',
snoozeButton: '#ring-button-snooze',
closeButton: '#ring-button-stop',
Expand Down
62 changes: 6 additions & 56 deletions apps/clock/js/utils.js
Expand Up @@ -229,19 +229,15 @@ Utils.escapeHTML = function(str, escapeQuotes) {
return span.innerHTML;
};

Utils.is12hFormat = function() {
var localeTimeFormat = mozL10n.get('dateTimeFormat_%X');
var is12h = (localeTimeFormat.indexOf('%p') >= 0);
return is12h;
Utils.getLocalizedTimeHtml = function(date) {
var f = new mozL10n.DateTimeFormat();
var shortFormat = mozL10n.get('shortTimeFormat');
return f.localeFormat(date, shortFormat.replace('%p', '<small>%p</small>'));
};

Utils.getLocaleTime = function(d) {
Utils.getLocalizedTimeText = function(date) {
var f = new mozL10n.DateTimeFormat();
var is12h = Utils.is12hFormat();
return {
time: f.localeFormat(d, (is12h ? '%I:%M' : '%H:%M')).replace(/^0/, ''),
ampm: is12h ? f.localeFormat(d, '%p') : ''
};
return f.localeFormat(date, mozL10n.get('shortTimeFormat'));
};

Utils.changeSelectByValue = function(selectElement, value) {
Expand All @@ -260,25 +256,6 @@ Utils.getSelectedValueByIndex = function(selectElement) {
return selectElement.options[selectElement.selectedIndex].value;
};

Utils.parseTime = function(time) {
var parsed = time.split(':');
var hour = +parsed[0]; // cast hour to int, but not minute yet
var minute = parsed[1];

// account for 'AM' or 'PM' vs 24 hour clock
var periodIndex = minute.indexOf('M') - 1;
if (periodIndex >= 0) {
hour = (hour == 12) ? 0 : hour;
hour += (minute.slice(periodIndex) == 'PM') ? 12 : 0;
minute = minute.slice(0, periodIndex);
}

return {
hour: hour,
minute: +minute // now cast minute to int
};
};

var wakeTarget = {
requests: {
cpu: new Map(), screen: new Map(), wifi: new Map()
Expand Down Expand Up @@ -381,33 +358,6 @@ Utils.repeatString = function rep(str, times) {
};

Utils.format = {
time: function(hour, minute, opts) {
var period = '';
opts = opts || {};
opts.meridian = typeof opts.meridian === 'undefined' ? true : opts.meridian;
var padHours = typeof opts.padHours === 'undefined' ? false : opts.padHours;
opts.padHours = padHours;

if (opts.meridian && Utils.is12hFormat()) {
period = hour < 12 ? 'AM' : 'PM';
hour = hour % 12;
hour = (hour === 0) ? 12 : hour;
}

if (opts.padHours && hour < 10) {
hour = '0' + hour;
}

if (hour === 0) {
hour = '00';
}

if (minute < 10) {
minute = '0' + minute;
}

return hour + ':' + minute + period;
},
hms: function(sec, format) {
var hour = 0;
var min = 0;
Expand Down
1 change: 0 additions & 1 deletion apps/clock/onring.html
Expand Up @@ -19,7 +19,6 @@
<div id="ring-clock">
<div id="ring-clock-display">
<span id="ring-clock-time"></span>
<span id="ring-clock-hour24-state"></span>
</div>
<div id="ring-label"></div>
</div>
Expand Down
7 changes: 2 additions & 5 deletions apps/clock/style/alarm.css
Expand Up @@ -115,9 +115,7 @@ ul#alarms li.alarm-cell > a:active {
background-color: rgba(52,140,158, .6);
}

ul#alarms li.alarm-cell > a:active .period,
ul#alarms li.alarm-cell > a:active .label,
ul#alarms li.alarm-cell > a:active .repeat {
ul#alarms li.alarm-cell > a:active {
color: #cfe2e5;
}

Expand Down Expand Up @@ -183,11 +181,10 @@ a.alarm-item > .time {
color: #cfe2e5;
}

a.alarm-item .period {
a.alarm-item small {
font-size: 1.4rem;
font-weight: normal;
color: #5C6466;
margin-left: 0.2rem;
}

a.alarm-item > .label {
Expand Down
2 changes: 1 addition & 1 deletion apps/clock/style/clock.css
Expand Up @@ -32,7 +32,7 @@ html, body {
pointer-events: none;
}

#clock-hour24-state {
#clock-time small {
font-weight: lighter;
font-size: 3.7rem;
color: #5C6466;
Expand Down
7 changes: 3 additions & 4 deletions apps/clock/style/onring.css
Expand Up @@ -56,11 +56,10 @@ html.ready {
width: 100%;
}

#ring-clock-hour24-state {
#ring-clock-time small {
font-weight: lighter;
font-size: 3.13rem;
font-size: inherit;
line-height: 5rem;
padding-left: 0.94rem;
}

#ring-label {
Expand Down Expand Up @@ -207,7 +206,7 @@ html.ready {
line-height: 1.38rem;
}

#ring-clock-hour24-state {
#ring-clock-time small {
display: none;
}

Expand Down
11 changes: 3 additions & 8 deletions apps/clock/test/unit/panels/alarm/clock_view_test.js
Expand Up @@ -78,31 +78,26 @@ suite('ClockView', function() {

teardown(function() {
ClockView.time.innerHTML = '';
ClockView.hourState.innerHTML = '';
this.clock.restore();
});

test('time and hourState elements are updated immediately',
test('time element is updated immediately',
function() {
ClockView.updateDigitalClock();
assert.equal(Date.parse(ClockView.time.innerHTML), this.sixAm + 1000);
assert.equal(ClockView.hourState.innerHTML, '&nbsp;&nbsp;');
});

test('time and hourState elements are not updated twice in the same ' +
'minute', function() {
test('time element is not updated twice in the same minute', function() {
ClockView.updateDigitalClock();
this.clock.tick(59 * 1000 - 1);
assert.equal(Date.parse(ClockView.time.innerHTML), this.sixAm + 1000);
assert.equal(ClockView.hourState.innerHTML, '&nbsp;&nbsp;');
});

test('time and hourState elements are updated each minute', function() {
test('time element is updated each minute', function() {
ClockView.updateDigitalClock();
this.clock.tick(59 * 1000);
assert.equal(Date.parse(ClockView.time.innerHTML),
this.sixAm + 60 * 1000);
assert.equal(ClockView.hourState.innerHTML, '&nbsp;&nbsp;');
});

});
Expand Down