1337319 - [DateTimePicker] The order of year and month on date picker should depend on locale datetime format

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Scott Wu [:scottwu]]

The order of month and year picker should be locale dependent. For example, the format for en-US should be month-year, but zh-TW should be year-month.

Comment 1

[Scott Wu [:scottwu]]

Attachment: Bug 1337319 - Order month and year spinners based on locale datetime format

Review commit: https://reviewboard.mozilla.org/r/113074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113074/

Comment 2

[Scott Wu [:scottwu]]

Comment on attachment 8838080 [details]
Bug 1337319 - Order month and year spinners based on locale datetime format

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113074/diff/1-2/

Comment 3

[Scott Wu [:scottwu]]

This patch uses Intl.DateTimeFormat's formatToParts API to get the order of year and month, then orders the spinners accordingly with `order` in CSS.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8838080 [details]
Bug 1337319 - Order month and year spinners based on locale datetime format

https://reviewboard.mozilla.org/r/113074/#review115022

r=me with the below changes. Thanks!

::: toolkit/content/widgets/datepicker.js:328
(Diff revision 2)
>          },
>          getDisplayString: year => yearFormat(new Date(new Date(0).setFullYear(year))),
>          viewportSize: spinnerSize
>        }, context.monthYearView)
>      };
> +    context.monthYearView.classList.add(spinnerOrder);

I wonder if it might be better to put this change _above_ the point where we attach the nodes (so above the construction of the Spinner's), so that we avoid having to recalculate their positions a second time.

::: toolkit/content/widgets/spinner.js:78
(Diff revision 2)
>        this.elements.spinner.style.height = (ITEM_HEIGHT * viewportSize) + "rem";
>  
>        if (hideButtons) {
>          this.elements.container.classList.add("hide-buttons");
>        }
> +      this.elements.container.id = id;

Let's only set this if `id` is not false-y.

Comment 5

[Scott Wu [:scottwu]]

Comment on attachment 8838080 [details]
Bug 1337319 - Order month and year spinners based on locale datetime format

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113074/diff/2-3/

Comment 6

[Scott Wu [:scottwu]]

Comment on attachment 8838080 [details]
Bug 1337319 - Order month and year spinners based on locale datetime format

https://reviewboard.mozilla.org/r/113074/#review115022

I've made the changes as suggested. Thanks!

> I wonder if it might be better to put this change _above_ the point where we attach the nodes (so above the construction of the Spinner's), so that we avoid having to recalculate their positions a second time.

Yeah that's a good idea.

> Let's only set this if `id` is not false-y.

Ok. Thanks for catching this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b02cdbaa32e
Order month and year spinners based on locale datetime format r=mconley

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey scottwu - just a heads up that I believe you have the ability to autoland via MozReview. After you've got r+'s, in the "Automation" dropdown, there should be a "Land Commits" option that will attempt to autoland your patches for you. That way, you don't need to set checkin-needed.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b02cdbaa32e