1470196 - Show the month name after the month number in the cc-exp-month dropdown

Status: VERIFIED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Eric Pang [:epang] UX]

Change format of month is expiry dropdown

Comment 1

[Eric Pang [:epang] UX]

Attachment: Expiry.png

Format month of expiry as number then text.
ex. '01 January' 

Also can be found here:
https://mozilla.invisionapp.com/d/main#/console/13422080/304878922/preview

Comment 2

[Matthew N. [:MattN]]

We can use `new Intl.DateTimeFormat(locale, { month: "long" })`

Comment 3

[Diego Pino]

Attachment: Bug-1470196-Show-the-month-name-after-the-month-numb.patch

Comment 4

[Diego Pino]

Attachment: Bug-1470196-Show-the-month-name-after-the-month-numb.patch

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 9026629 [details] [diff] [review]
Bug-1470196-Show-the-month-name-after-the-month-numb.patch

Review of attachment 9026629 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Diego. Can you also make sure that the Payment Request and autofill tests pass with this change:

./mach test browser/components/payments/
./mach mochitest browser/extensions/formautofill/

::: browser/components/payments/res/containers/basic-card-form.js
@@ +264,5 @@
>      // billingAddressSelect is updated after loadRecord.
>      this.formHandler.updatePopulatedState(billingAddressSelect.popupBox);
>  
> +    // Fill up cc-expire-month options. Format: "month number - month name".
> +    let dateFormat = new Intl.DateTimeFormat(navigator.language, {month: 'long'}).format;

Sorry, I should have been more explicit that you should do this in https://searchfox.org/mozilla-central/rev/8f89901f2d69d9783f946a7458a6d7ee70635a94/browser/extensions/formautofill/content/autofillEditForms.js#342,348,372 so it also affects the preferences dialog. You can add a helper like `generateYears` which gets called from the constructor (not loadRecord since you're only changing the label).

@@ +269,5 @@
> +    let ccExpMonthOptions = document.querySelectorAll('#cc-exp-month > option');
> +    for (let option of ccExpMonthOptions) {
> +      if (option.value.length > 0) {
> +        let value = Number.parseInt(option.value);
> +        let monthName = dateFormat(new Date(Date.UTC(1970, value-1, 1)));

Nit: spaces around operators like '-'

Comment 6

[Diego Pino]

Attachment: Bug-1470196-Show-the-month-name-after-the-month-numb.patch

I reworked the code to make it more similar to `generateYears` (now `generateMonths` creates the options and appends them to the list), so they can be placed at the same level.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 9026910 [details] [diff] [review]
Bug-1470196-Show-the-month-name-after-the-month-numb.patch

Review of attachment 9026910 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good with some small changes assuming the tests all pass.

::: browser/extensions/formautofill/content/autofillEditForms.js
@@ +380,5 @@
> +    // Empty month option
> +    this._elements.month.appendChild(new Option());
> +
> +    // Populate cc-expire-month list. Format: "month number - month name"
> +    let dateFormat = new Intl.DateTimeFormat(navigator.language, {month: 'long'}).format;

Nit: use double-quotes for Mozilla JS style

@@ +387,5 @@
> +        let monthName = dateFormat(new Date(Date.UTC(1970, i, 1)));
> +        let option = new Option();
> +        option.value = monthNumber;
> +        monthNumber = monthNumber < 10 ? "0" + monthNumber : monthNumber.toString();
> +        option.textContent = `${monthNumber} - ${monthName}`;

It's a bit unusual to change the type of monthNumber from a Number to a String. Can you avoid that please? You can also use padStart:

let monthNumber = (i + 1).toString();
option.value = monthNumber;
option.textContent = `${monthNumber.padStart(2, "0")} - ${monthName}`;

@@ +388,5 @@
> +        let option = new Option();
> +        option.value = monthNumber;
> +        monthNumber = monthNumber < 10 ? "0" + monthNumber : monthNumber.toString();
> +        option.textContent = `${monthNumber} - ${monthName}`;
> +        this._elements.month.appendChild(option);

We also should be localizing the hyphen and doing substitution so a locale can re-order the string but that is quite annoying until we have bug 1446164 so I've just noted that issue there. Can you add a comment:
// XXX: Bug 1446164 - Localize this string.

Comment 8

[Diego Pino]

Attachment: Bug-1470196-Show-the-month-name-after-the-month-numb.patch

Addressed comments from latest review.

Comment 9

[Diego Pino]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=81eb1b41859893c966a79bf29895dcded7ba69a0

Comment 10

[Diego Pino]

Attachment: Bug-1470196-Show-the-month-name-after-the-month-numb.patch

Fixed error in previous patch (months were only generated until November).

Comment 11

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd99fb8a0bc7cddbe17b463c570ec2dc432d766f

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 9027107 [details] [diff] [review]
Bug-1470196-Show-the-month-name-after-the-month-numb.patch

Review of attachment 9027107 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfdd8cc5f88a
Show the month name after the month number in the cc-exp-month dropdown r=MattN

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/bfdd8cc5f88a

Comment 15

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 65.0a1 (2018-12-03) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.