-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for 12-hour clock (AM/PM) for TimePicker component #2151
Conversation
FYI, I am currently in progress on adding test cases and linting so that CI will accept this branch. |
@colinbr96 amazing work! Thanks in advance for tests. API-wise, I'm not a fan of the two strings Finally, would you kindly configure your circle build so the preview comment will post correctly? There's a wiki page with instructions for the environment variable. |
Changed API to useAmPm propPreview: documentation | landing | table |
I have updated the API to use a boolean instead. Now the TimePicker prop will be called |
Added tests for remaining dateUtils functionsPreview: documentation | landing | table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colinbr96 sorry to leave you hanging for so long!
this looks pretty good, and the approach is solid. here's a hefty code review to get this feature to the next level! 🌴
@@ -73,6 +74,10 @@ $timepicker-control-width: $pt-grid-size * 3.3 !default; | |||
} | |||
} | |||
|
|||
.pt-timepicker-ampm-select { | |||
margin-left: 2px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$pt-grid-size / 2
please: typical small spacing.
if (hour < 0 || hour > 23) { | ||
throw new Error(`hour must be between [0,23] inclusive: got ${hour}`); | ||
} | ||
hour = hour % 12; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙅♂️ please refrain from modifying arguments. create new const
variables.
if (hour === 0) { | ||
hour = 12; | ||
} | ||
return hour; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we love ternary statements here: return hour === 0 ? 12 : hour;
|
||
export function get24HourFrom12Hour(hour: number, isPm: boolean): number { | ||
if (hour < 1 || hour > 12) { | ||
throw new Error(`hour must be between [0,12] inclusive: got ${hour}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
< 1
and [0,12]
don't match up
throw new Error(`hour must be between [0,12] inclusive: got ${hour}`); | ||
} | ||
if (isPm) { | ||
hour = hour + 12; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid modifying arguments. new variables please.
packages/datetime/src/timePicker.tsx
Outdated
@@ -375,3 +439,8 @@ function handleKeyEvent(e: React.KeyboardEvent<HTMLInputElement>, actions: IKeyE | |||
} | |||
} | |||
} | |||
|
|||
/** Event handler that exposes the target element's value as a string. */ | |||
function handleStringChange(handler: (value: string) => void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this. it's my favorite util function, too, but it does not belong here.
🤔 we should move them from docs-theme to core utils, in a future PR. #helpwanted 😄
packages/datetime/src/timePicker.tsx
Outdated
</option> | ||
<option key={1} value={"pm"}> | ||
PM | ||
</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key
is only necessary when rendering an array. refactor these to: <option value="pm">PM</option>
packages/datetime/src/timePicker.tsx
Outdated
const didUseAmPmChange = nextProps.useAmPm !== this.props.useAmPm; | ||
|
||
let value = this.state.value; | ||
let useAmPm = this.props.useAmPm || TimePicker.defaultProps.useAmPm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the || defaultProps
here. React already does that for us 😄
packages/datetime/src/timePicker.tsx
Outdated
}; | ||
|
||
public static displayName = "Blueprint2.TimePicker"; | ||
|
||
public constructor(props?: ITimePickerProps, context?: any) { | ||
super(props, context); | ||
|
||
let value = props.minTime; | ||
let useAmPm = TimePicker.defaultProps.useAmPm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not necessary. props.useAmPm
will already be set to this. you can remove this variable entirely.
packages/datetime/src/timePicker.tsx
Outdated
} | ||
|
||
this.state = this.getFullStateFromValue(value, useAmPm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just pass props.useAmPm
along here
Thanks for the code review. I got the requested changes completed. Hopefully it's better this time! 🕰️ |
Remove explicit references to defaultProps for useAmPmPreview: documentation | landing | table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, now we're cooking with 🔥
packages/datetime/src/timePicker.tsx
Outdated
@@ -329,6 +307,18 @@ export class TimePicker extends React.Component<ITimePickerProps, ITimePickerSta | |||
} | |||
}; | |||
|
|||
private handleAmPmChange = (e: React.SyntheticEvent<HTMLSelectElement>) => { | |||
const changeToAm = e.currentTarget.value === "am"; | |||
if (changeToAm !== this.state.isPm) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comparing am
to pm
seems off. make both checks about pm
.
if (isNextPm !== this.state.isPm) {
const hour = ...
// logic inside conditional
}
packages/datetime/src/timePicker.tsx
Outdated
return; | ||
} | ||
const hour = DateUtils.convert24HourMeridiem(this.state.value.getHours(), changeToAm); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line, the next block follows directly from variable assignment.
packages/datetime/src/timePicker.tsx
Outdated
|
||
this.setState({ isPm: !changeToAm }, () => { | ||
this.updateTime(hour, TimeUnit.HOUR_24); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inline short functions like this. this.setState({ props }, () => this.updateTime())
expect(DateUtils.getIsPmFrom24Hour(12)).to.equal(true); | ||
expect(DateUtils.getIsPmFrom24Hour(23)).to.equal(true); | ||
|
||
for (let hour = 12; hour <= 23; hour++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait wait the point of my previous comment was to replace all for
loops with a handful of explicit tests. you added explicit tests but didn't remove the loops.
no need to test 12 and 23 and then test 12 through 23.
(this comment applies to every test in this file. i want to see zero for
loops when we're done.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to explicitly test for all values from 12 to 23?
e.g.
expect(DateUtils.getIsPmFrom24Hour(12)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(13)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(14)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(15)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(16)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(17)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(18)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(19)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(20)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(21)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(22)).to.equal(true);
expect(DateUtils.getIsPmFrom24Hour(23)).to.equal(true);
Normally I would just iterate this
packages/datetime/src/timePicker.tsx
Outdated
PM | ||
</option> | ||
<option value={"am"}>AM</option> | ||
<option value={"pm"}>PM</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
braces are not necessary for string literals. value="pm"
packages/datetime/src/timePicker.tsx
Outdated
this.setState(this.getFullStateFromValue(nextProps.value)); | ||
value = nextProps.value; | ||
} | ||
if (didUseAmPmChange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this check is useless and it is equivalent to just passing through nextProps.useAmPm
. if props are unchanged then next is the same as this.props
. please remove (just like we did on line 130).
packages/datetime/src/timePicker.tsx
Outdated
@@ -64,6 +64,12 @@ export interface ITimePickerProps extends IProps { | |||
*/ | |||
showArrowButtons?: boolean; | |||
|
|||
/** | |||
* Whether to choose from a 12 hour format with an AM/PM dropdown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Whether to [use a] 12-hour format with an AM/PM dropdown."
|
||
describe("convert24HourMeridiem", () => { | ||
it("returns given hour, if hour is AM and toAm", () => { | ||
for (let hour = 0; hour <= 11; hour++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment still stands. please address it in all tests.
|
||
for (let hour = 12; hour <= 23; hour++) { | ||
expect(DateUtils.getIsPmFrom24Hour(hour)).to.equal(true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you added some explicit values, now it's time to remove the for
loop.
(this applies to several tests here)
Fix misc code review requestsPreview: documentation | landing | table |
@giladgray For some reason this pull request still says "changes requested" however I have already finished making the requested changes 3 days ago. Not sure what I need to do next. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks good, thanks @colinbr96!!
Fixes #1409
Checklist
Changes proposed in this pull request:
Adding an additional prop
hourFormat
useAmPm
to the TimePicker component to support a 12-hour clock and AM/PM switcher.hourFormat
can be set to either of the two strings"12hour"
or"24hour"
to toggle the AM/PM switcher and type of hour unit being used.useAmPm
is a boolean which defaults tofalse
(using the 24-hour clock by default to maintain backwards compatibility) and can be set totrue
to set the hour range to 12-hours and display an AM/PM dropdown.I also added a switch to the TimePicker documentation page to set the example component's
hourFormat
.Reviewers should focus on:
Video Demo
https://media.giphy.com/media/39m89EvdXoQFbCPKYk/giphy.gif