[Labs] Timezone Picker #1568

brieb · 2017-09-14T19:24:21Z

Fixes part of #890

Out of scope for this PR: Incorporating with the other datetime components

Checklist

Include tests
Update documentation

Changes proposed in this pull request:

Timezone select component

…ings

blueprint-bot · 2017-09-19T04:55:32Z

Docs nits

Preview: documentation | table
Coverage: core | datetime

llorca

So. Good.

blueprint-bot · 2017-09-19T17:55:42Z

Fix lints

Preview: documentation | table
Coverage: core | datetime

blueprint-bot · 2017-09-19T22:03:19Z

Timezone picker tests

Preview: documentation | table
Coverage: core | datetime

giladgray

(check all my comments--not sure they'll all appear on the diff cuz I created them from the "View changes" partial diff view)

giladgray · 2017-09-20T21:49:06Z

packages/labs/examples/timezonePickerExample.tsx

-    private handleShowLocalTimezoneChange = handleBooleanChange((showLocalTimezone) =>
-        this.setState({ showLocalTimezone }));
+    private handleDisabledChange = handleBooleanChange(disabled => this.setState({ disabled }));
+    private handleShowLocalTimezoneChange = handleBooleanChange(showLocalTimezone =>


please always wrap params in parens. this should be a lint failure...

no, this is the prettier standard

giladgray · 2017-09-20T21:51:10Z

packages/labs/src/components/select/select.tsx

+    public componentWillReceiveProps(nextProps: ISelectProps<T>) {
+        const { inputProps: nextInputProps = {} } = nextProps;
+        if (nextInputProps.value !== undefined && this.state.query !== nextInputProps.value) {
+            this.setState({ query: nextInputProps.value });


nice! this seems like a valuable fix and could use a unit test (since Select has actual coverage).

in fact you could go so far as to pull this change to a separate PR focused solely on supporting controlled input value.

though in truth we probably want an obvious query prop instead of inputProps.value?

Going to start on this in a separate PR

giladgray · 2017-09-20T21:52:25Z

packages/labs/src/components/timezone-picker/timezoneDisplayFormat.ts

@@ -32,7 +32,7 @@ export function formatTimezone(
    const { abbreviation, offsetAsString } = getTimezoneMetadata(timezone, date);
    switch (displayFormat) {
        case TimezoneDisplayFormat.ABBREVIATION:
-            // Fall back to the offset in regions where there is no abbreviation.
+            // Fall back to the offset when there is no abbreviation.
            return abbreviation ? abbreviation : offsetAsString;


prefer to avoid implicit boolean coercion. abbreviation === undefined ? default : actual

giladgray · 2017-09-20T21:52:42Z

packages/labs/src/components/timezone-picker/timezoneItems.ts

    key: string;
+    /** Text for the timezone. */


blank line after each property, before the next doc comment.

giladgray · 2017-09-20T21:53:54Z

packages/labs/src/components/timezone-picker/timezoneMetadata.ts

@@ -23,9 +23,7 @@ export function getTimezoneMetadata(timezone: string, date: Date): ITimezoneMeta
    const offsetAsString = zonedDate.format("Z");
    const abbreviation = getAbbreviation(timezone, timestamp);
    // TODO: amend moment-timezone typings to include the population field


issue to track this? in moment's repo

DefinitelyTyped/DefinitelyTyped#19947

giladgray · 2017-09-20T21:57:31Z

packages/labs/src/components/timezone-picker/timezoneUtils.ts

+            filteredItems.push(item);
+            seenItem[key] = true;
+        }
+    }


please add code comments explaining what lines 62-75 are doing... I can't follow and I don't trust it.

giladgray · 2017-09-20T21:58:33Z

packages/labs/test/timezonePickerTests.tsx

+    it("clicking on button target opens popover", () => {
+        const timezonePicker = mount(<TimezonePicker popoverProps={{ inline: true }} />);
+        const button = timezonePicker.find(Button);
+        button.simulate("click");


if you don't need a reference later then no need to make a variable.

timezonePicker.find(Button).simulate("click");

giladgray · 2017-09-20T21:59:29Z

packages/labs/test/timezonePickerTests.tsx

+    it("placeholder prop changes the filter placeholder text", () => {
+        const placeholder = "test placeholder";
+        const timezonePicker = shallow(
+            <TimezonePicker popoverProps={{ isOpen: true, inline: true }} inputProps={{ placeholder }} />,


popoverProps={POPOVER_PROPS} one object to rule them all?

giladgray · 2017-09-20T22:00:39Z

packages/labs/test/timezonePickerTests.tsx

+        assert.strictEqual(firstItem.timezone, moment.tz.guess());
+    });
+
+    it("does not render local timezone at top of item list if show local timezone is disabled", () => {


use prop name in test name: if showLocalTimezone=false
this comment applies widely to this file.

giladgray · 2017-09-20T22:04:51Z

packages/labs/src/components/timezone-picker/timezone-picker.md

+@## JavaScript API
+
+[Moment Timezone](http://momentjs.com/timezone/) is used internally
+for the list of available timezones and timezone metadata.


this line belongs at the end of the section, not the beginning: it's an implementation detail, not relevant to usage.

cmslewis · 2017-09-21T00:08:16Z

On filter, can we scroll the matched item to the top of the list? This is what's happening now:

Also, EST doesn't suggest EST, but rather EST5EDT. I'd expect fuzz-aldrin to return the former, no?

Add link to PR for official typings

brieb · 2017-09-21T21:57:28Z

@cmslewis Ya, it appears that select has the behavior that if an earlier query matched an item (est5edt in your case), that value stays selected even when a longer query has a different first item. I would expect the first item slot to remain active instead of jumping half-way down the list. We should distinguish between whether the user actually activated an item vs us just activating the first item by default.

Made it so est shows up first. 2b37613

blueprint-bot · 2017-09-21T22:01:24Z

Fix tests

Preview: documentation | table
Coverage: core | datetime

blueprint-bot · 2017-09-21T22:25:55Z

Concatenate item query candidates for better ranking

Preview: documentation | table
Coverage: core | datetime

addressed comments

Brie Bunge added 30 commits September 12, 2017 10:12

Remove non-existent tsconfig include dir

356743c

Install moment and moment-timezone

66c6edc

Remove unused import

4a4aef2

Shorten imports

b6d8302

Timezone Input V0

456303a

Use Select component

7de36a1

Seed list with representative timezones

67ca676

Additional timezone metadata

dad05d1

Specify general tab size and scss-specific tab size in workspace sett…

75dd866

…ings

Left-aligned time zone offset label

a1ee5cb

Labels look better on right

5739478

Enforce a min-width, so the menu doesn't jump around while filtering

f61607c

Move abbreviation after name, so offsets align

185a671

Additional props: disabled, defaultTimezone, selectedTimezoneFormat

4bb8e12

Clean up naming and initial timezone getter

50f8e78

showUserTimezoneGuess prop

0b0237c

Pass through popoverProps

25176ab

placeholder prop

c08169f

targetClassName prop

3e4a482

Add disabled switch to example; Use handler helpers

8823063

Add interface to docs page

4c2def9

Add example switches for default timezone and use guess

e5dc667

Timezone query candidates

ba10a57

Change empty state text

86ade6b

Only show user timezone if query is empty

cbf7ee3

Controlled mode for selected timezone

72c143a

Don't exclude popular guess

cd0f2ad

Add onQueryChange to account for the query being changed through reset

8671cc0

Use onQueryChange

d0f1e4f

Add defaultToUserTimezoneGuess prop

f6f8b93

Docs nits

ab8d5d5

brieb changed the title ~~⚠️ [WIP] Timezone Select~~ [Labs] Timezone Picker Sep 19, 2017

llorca approved these changes Sep 19, 2017

View reviewed changes

Brie Bunge added 3 commits September 19, 2017 10:41

Merge remote-tracking branch 'origin/master' into bb/timezone

b8e6c32

Make timezone code "prettier"

68ce37c

Fix lints

7f5df58

Brie Bunge added 4 commits September 19, 2017 13:42

[Labs/Select] Sync input props value with query value

b0be6b6

[Labs/TimezonePicker] Sync input props value with query value

ffbb9d3

Fix misnamed test

956b612

Timezone picker tests

76fccc2

giladgray previously requested changes Sep 20, 2017

View reviewed changes

Brie Bunge added 6 commits September 21, 2017 13:05

Fix nits

c208968

Use module augmentation instead of casting for zone.population

56bf58e

Add link to PR for official typings

Don't import from ".."

d5ab205

Fix doc nits

3453ce0

Make test style more consistent

b14a7c2

Fix tests

248f670

Concatenate item query candidates for better ranking

2b37613

adidahiya approved these changes Sep 22, 2017

View reviewed changes

adidahiya merged commit 69fa528 into master Sep 22, 2017

adidahiya deleted the bb/timezone branch September 22, 2017 16:05

llorca mentioned this pull request Sep 22, 2017

Prepare release v1.29.0 #1602

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Labs] Timezone Picker #1568

[Labs] Timezone Picker #1568

brieb commented Sep 14, 2017 •

edited

Loading

blueprint-bot commented Sep 19, 2017

llorca left a comment

blueprint-bot commented Sep 19, 2017

blueprint-bot commented Sep 19, 2017

giladgray left a comment

giladgray Sep 20, 2017

adidahiya Sep 20, 2017

giladgray Sep 20, 2017

brieb Sep 21, 2017

brieb Sep 22, 2017

giladgray Sep 20, 2017

giladgray Sep 20, 2017

giladgray Sep 20, 2017

brieb Sep 21, 2017

giladgray Sep 20, 2017

giladgray Sep 20, 2017

giladgray Sep 20, 2017

giladgray Sep 20, 2017

giladgray Sep 20, 2017

cmslewis commented Sep 21, 2017

brieb commented Sep 21, 2017 •

edited

Loading

blueprint-bot commented Sep 21, 2017

blueprint-bot commented Sep 21, 2017

[Labs] Timezone Picker #1568

[Labs] Timezone Picker #1568

Conversation

brieb commented Sep 14, 2017 • edited Loading

Fixes part of #890

Checklist

Changes proposed in this pull request:

blueprint-bot commented Sep 19, 2017

Docs nits

llorca left a comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 19, 2017

Fix lints

blueprint-bot commented Sep 19, 2017

Timezone picker tests

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis commented Sep 21, 2017

brieb commented Sep 21, 2017 • edited Loading

blueprint-bot commented Sep 21, 2017

Fix tests

blueprint-bot commented Sep 21, 2017

Concatenate item query candidates for better ranking

brieb commented Sep 14, 2017 •

edited

Loading

brieb commented Sep 21, 2017 •

edited

Loading