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

RESOLVED FIXED in Firefox 54

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks: 1 bug)

54 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
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.
(Assignee)

Updated

4 months ago
Assignee: nobody → scwwu
Component: XUL Widgets → Layout: Form Controls
Product: Toolkit → Core
Version: unspecified → 54 Branch
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

4 months ago
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

4 months ago
mozreview-review
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.
Attachment #8838080 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

4 months ago
mozreview-review-reply
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.
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 7

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8b02cdbaa32e
Order month and year spinners based on locale datetime format r=mconley
Keywords: checkin-needed
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b02cdbaa32e
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.