Closed Bug 1374967 Opened 7 years ago Closed 7 years ago

[DateTimeInput] Consider also @step when deciding to show or hide second/millisecond field

Categories

(Core :: Layout: Form Controls, enhancement)

Product:
Core
Component:
Layout: Form Controls
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: jessica, Assigned: jessica)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Part 1: Add helper functions to know if second/millisecond/day period exists.
7.86 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 2: Consider step when deciding whether to show second/millisecond field, v1.
22.87 KB, patch
jessica
: review+
Details | Diff | Splinter Review
Currently, we only show second/millisecond field when @value has second/millisecond part. But actually, we should also consider @step, for example, if step is "0.001", then user should be able to change the millisecond field. Another improvement is to enable/disable fields based on step, for example, if step is 3600 (seconds), then the minute can be disabled, since changing the minute field would be invalid. This could be done in another bug.
Assignee: nobody → jjong
We should consider step and step base when deciding whether to show second and millisecond field, since step and step base can affect the valid time intervals, and the valid intervals may have second/millsecond part.
Attachment #8881973 - Flags: review?(bugs)
Attachment #8881970 - Flags: review?(bugs) → review+
Comment on attachment 8881973 [details] [diff] [review] Part 2: Consider step when deciding whether to show second/millisecond field. ># HG changeset patch ># Parent bb194483c8ecf2c7bd47dc57dd94182cba7efe50 >Bug 1374967 - Part 2: Consider step when deciding whether to show second/millisecond field. r=smaug > >We should consider step and step base when deciding whether to show second and >millisecond field, since step and step base can affect the valid time intervals, >and the valid intervals may have second/millsecond part. millisecond >+ <method name="reBuildEditFieldsIfNeeded"> I think it is usually rebuild, not reBuild >- // Second and millsecond part is optional, rebuild edit fields if >+ // Second and millsecond part are optional, rebuild edit fields if millisecond > // needed. >- if ((this.isEmpty(second) == this.hasSecondField()) || >- (this.isEmpty(millisecond) == this.hasMillisecField())) { >- this.log("Edit fields need to be rebuilt.") >- this.reBuildEditFields(); oh, reBuild is existing style here. ok fine. >+ <method name="notifyMinMaxStepAttrChanged"> >+ <body> >+ <![CDATA[ >+ // Second and millsecond part are optional, rebuild edit fields if millisecond Do we have tests when step is something weird, where it ends up increasing/decreasing by, say 61 seconds, or 1001 milliseconds?
Attachment #8881973 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #3) > Comment on attachment 8881973 [details] [diff] [review] > Part 2: Consider step when deciding whether to show second/millisecond field. > > ># HG changeset patch > ># Parent bb194483c8ecf2c7bd47dc57dd94182cba7efe50 > >Bug 1374967 - Part 2: Consider step when deciding whether to show second/millisecond field. r=smaug > > > >We should consider step and step base when deciding whether to show second and > >millisecond field, since step and step base can affect the valid time intervals, > >and the valid intervals may have second/millsecond part. > millisecond will do. > >+ <method name="reBuildEditFieldsIfNeeded"> > I think it is usually rebuild, not reBuild yeah, "rebuild" looks better, will change them all. > > >- // Second and millsecond part is optional, rebuild edit fields if > >+ // Second and millsecond part are optional, rebuild edit fields if > millisecond will do. > > > // needed. > >- if ((this.isEmpty(second) == this.hasSecondField()) || > >- (this.isEmpty(millisecond) == this.hasMillisecField())) { > >- this.log("Edit fields need to be rebuilt.") > >- this.reBuildEditFields(); > oh, reBuild is existing style here. ok fine. > > >+ <method name="notifyMinMaxStepAttrChanged"> > >+ <body> > >+ <![CDATA[ > >+ // Second and millsecond part are optional, rebuild edit fields if > millisecond will do. > > Do we have tests when step is something weird, where it ends up > increasing/decreasing by, say 61 seconds, or 1001 milliseconds? Will add some more tests for this.
Fix typos and added some more tests, carrying r+.
Attachment #8881973 - Attachment is obsolete: true
Attachment #8882268 - Flags: review+
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0299924b1 Part 1: Add helper functions to know if second/millisecond/day period exists. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/876fba84db76 Part 2: Consider step when deciding whether to show second/millisecond field. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0d0299924b1 https://hg.mozilla.org/mozilla-central/rev/876fba84db76
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Verified as fixed using the latest Nightly 56.0a1 (2017-07-09) on Ubuntu 16.04, Mac OS X 10.12 and Windows 10 x64.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: