Create new class for DateRangePicker state internals #619

leebyp · 2017-02-04T02:53:53Z

The current way that we handle the displayMonth and in the state in DateRangePicker makes the code pretty unreadable if you do the obvious extension to the state as in #575.

Want to see how folks feel about creating a new class with all the comparison and helper methods. Caveat is that in the edge cases, you can end up cloning and recreating a lot of classes, but they're just functions anyway so shouldn't hit any perf problems.

This change along with the comments in the blocks from #575 should make things a lot easier to parse.

NB. Want some feedback if this is an awful idea before I clean up the duplication and un-disable the tests

leebyp · 2017-02-07T23:31:15Z

@palantir/blueprint i'm gonna keep going ahead along these lines if this looks good

cmslewis · 2017-02-08T00:20:48Z

packages/datetime/src/common/dateUtils.ts

+export function getDateNextMonth(date: Date): Date {
+    const nextMonth = getNextMonth([date.getMonth(), date.getFullYear()]);
+    return new Date(nextMonth[1], nextMonth[0]);
+}


nit: Missing newline?

cmslewis · 2017-02-08T00:22:11Z

packages/datetime/src/common/displayMonth.ts

+
+// returns 1 if left < right
+// returns -1 if left > right
+// returns 0 if left === right


This comparator scheme is backward, no? Should return -1 if the left one is lesser, +1 if the left one is greater.

I was going for "in-order is truthy", but agree that following standard convention is easier to understand. Updated

cmslewis · 2017-02-08T00:23:52Z

packages/datetime/src/common/dateUtils.ts

+
+type MonthAndYear = [number, number];
+
+function getPreviousMonth([month, year]: MonthAndYear): MonthAndYear {


Looks like this function isn't referenced anywhere. Do we still need it?

https://github.com/palantir/blueprint/pull/619/files#diff-1a29b9bf8cc52d21c15d52b7c7668f85R38

cmslewis · 2017-02-08T00:24:24Z

packages/datetime/src/common/dateUtils.ts

+  return month === Months.JANUARY ? [Months.DECEMBER, year - 1] : [month - 1, year];
+}
+
+function getNextMonth([month, year]: MonthAndYear): MonthAndYear {


https://github.com/palantir/blueprint/pull/619/files#diff-1a29b9bf8cc52d21c15d52b7c7668f85R43

cmslewis · 2017-02-08T00:25:49Z

packages/datetime/test/dateRangePickerTests.tsx

@@ -16,7 +16,7 @@ import * as Errors from "../src/common/errors";
 import { Months } from "../src/common/months";
 import { Classes as DateClasses, DateRange, DateRangePicker, IDateRangePickerProps } from "../src/index";

-describe("<DateRangePicker>", () => {
+xdescribe("<DateRangePicker>", () => {


Reasoning behind xdescribe vs. describe.skip? Reads more clearly if it'll behave equivalently for you.

i prefer describe.skip for readability. also we have lint rules banning describe.skip so you won't accidentally ~~commit~~ merge it.

oh nice, i can't find those rules - is it possible to also lint for the xit and xdescribe variants?

giladgray · 2017-02-08T21:59:37Z

packages/datetime/src/common/displayMonth.ts

+    private date: Date;
+
+    constructor (month: number, year: number) {
+        if (month !== null && year !== null) {


if they are optional then mark them as such.
constructor(month?: number, year?: number)

giladgray · 2017-02-08T22:00:37Z

packages/datetime/src/common/dateUtils.ts

+
+type MonthAndYear = [number, number];
+
+function getPreviousMonth([month, year]: MonthAndYear): MonthAndYear {


https://github.com/palantir/blueprint/pull/619/files#diff-1a29b9bf8cc52d21c15d52b7c7668f85R38

giladgray · 2017-02-08T22:00:51Z

packages/datetime/src/common/dateUtils.ts

+  return month === Months.JANUARY ? [Months.DECEMBER, year - 1] : [month - 1, year];
+}
+
+function getNextMonth([month, year]: MonthAndYear): MonthAndYear {


https://github.com/palantir/blueprint/pull/619/files#diff-1a29b9bf8cc52d21c15d52b7c7668f85R43

giladgray · 2017-02-08T22:01:42Z

packages/datetime/src/common/displayMonth.ts

+    }
+
+    public isAfter(displayMonth: DisplayMonth): boolean {
+        return compareDisplayMonth(this, displayMonth) === -1;


do > 0 and < 0--more readable and robust, as comparators can return other positive/negative values with same semantics.

giladgray · 2017-02-08T22:02:32Z

packages/datetime/src/common/displayMonth.ts

+
+import { Months } from "./months";
+
+export class DisplayMonth {


i might advocate for calling this MonthAndYear cuz that's what it is.

yup! made a big rename

giladgray · 2017-02-08T22:05:26Z

packages/datetime/src/dateRangePicker.tsx

-            rightDisplayMonth: rightDisplay[0],
-            rightDisplayYear: rightDisplay[1],
-        });
+    private setDisplay(leftDisplayMonth: DisplayMonth, rightDisplayMonth: DisplayMonth) {


this function doesn't seem very helpful anymore. could just inline it when used.

It's useful mainly so the setState in the other methods don't become a pain to read because they're named potentialXXXX. It's also nice to limit the places we have setState in this complicated component.

giladgray · 2017-02-08T22:05:36Z

packages/datetime/src/dateRangePicker.tsx

-        let potentialLeftDisplay: DisplayMonth = [state.leftDisplayMonth, state.leftDisplayYear];
-        let potentialRightDisplay: DisplayMonth = [state.rightDisplayMonth, state.rightDisplayYear];
+        let potentialLeftDisplay: DisplayMonth = state.leftDisplayMonth.clone();
+        let potentialRightDisplay: DisplayMonth = state.rightDisplayMonth.clone();


def don't need to explicitly type these vars

giladgray · 2017-02-08T22:06:00Z

packages/datetime/src/dateRangePicker.tsx

-                if (compareDisplayMonth(potentialLeftDisplay, potentialRightDisplay) !== DisplayMonthOrder.IN_ORDER) {
-                    potentialLeftDisplay = getPreviousMonth(potentialRightDisplay);
+            const nextValueEndMonthDisplay: DisplayMonth =
+                new DisplayMonth(nextValueEnd.getMonth(), nextValueEnd.getFullYear());


this looks like a .clone()

giladgray · 2017-02-08T22:06:29Z

packages/datetime/src/dateRangePicker.tsx

+            const nextValueStartMonthDisplay: DisplayMonth =
+                new DisplayMonth(nextValueStart.getMonth(), nextValueStart.getFullYear());
+            const nextValueEndMonthDisplay: DisplayMonth =
+                new DisplayMonth(nextValueEnd.getMonth(), nextValueEnd.getFullYear());


these are both .clones?

this and above are converting Dates to MonthAndYears

blueprint-bot · 2017-02-09T07:39:18Z

rename DisplayMonth to MonthAndYear

Preview: docs | landing | table
Coverage: core | datetime

blueprint-bot · 2017-02-09T07:49:50Z

switch displaymonth comparator to conventional return values

Preview: docs | landing | table
Coverage: core | datetime

giladgray · 2017-02-10T01:43:47Z

packages/datetime/src/common/monthAndYear.ts

+        if (firstMonth > secondMonth) {
+            return 1;
+        }
+    }


if (firstYear === secondYear) { return firstMonth - secondMonth; // positive number means first is larger } else { return firstYear - secondYear; // negative number means second is larger }

well aint that neat, updated!

giladgray · 2017-02-10T01:45:50Z

packages/datetime/src/dateRangePicker.tsx

@@ -415,53 +415,51 @@ export class DateRangePicker
    * and rectify appropriately.
    */
    private handleLeftYearSelectChange = (leftDisplayYear: number) => {
-        let potentialLeftDisplay = new DisplayMonth(this.state.leftDisplayMonth.getMonth(), leftDisplayYear);
+        let potentialLeftMonthAndYear = new MonthAndYear(this.state.leftMonthAndYear.getMonth(), leftDisplayYear);


ok we get it, there's a month and a year! 😛

giladgray · 2017-02-10T01:47:03Z

packages/datetime/src/dateRangePicker.tsx

-            leftDisplayMonth: potentialLeftDisplay,
-            rightDisplayMonth: potentialRightDisplay,
+            leftMonthAndYear: potentialLeftMonthAndYear,
+            rightMonthAndYear: potentialRightMonthAndYear,


but actually renaming all these variables makes this thing dang near impossible to read. what if all the variables called it a View instead of MonthAndYear? so much shorter and more digestible.

even better! the logic's pretty readable now

blueprint-bot · 2017-02-10T07:05:27Z

change DateRangePicker internal state name

Preview: docs | landing | table
Coverage: core | datetime

blueprint-bot · 2017-02-10T23:01:30Z

simplify compare month and year

Preview: docs | landing | table
Coverage: core | datetime

blueprint-bot · 2017-02-11T02:21:08Z

fix utils function

Preview: docs | landing | table
Coverage: core | datetime

Create new class for DateRangePicker state internals

f6675eb

llorca requested review from cmslewis and giladgray February 7, 2017 23:36

cmslewis reviewed Feb 8, 2017

View reviewed changes

giladgray suggested changes Feb 8, 2017

View reviewed changes

leebyp added 3 commits February 8, 2017 22:41

cleanup

2baed57

fix tests

dae9744

rename DisplayMonth to MonthAndYear

9723e3c

switch displaymonth comparator to conventional return values

bca80a1

giladgray reviewed Feb 10, 2017

View reviewed changes

change DateRangePicker internal state name

f449a86

simplify compare month and year

25f2366

fix utils function

fa01e09

leebyp merged commit cc3d5e4 into bl/independent-daterangepicker-months Feb 13, 2017

leebyp deleted the bl/daterangepicker-state-refactor branch February 13, 2017 19:31

blueprint-bot mentioned this pull request Feb 13, 2017

Independent Daterangepicker months #575

Merged

3 tasks

snyk-bot mentioned this pull request Apr 24, 2023

[Snyk] Fix for 1 vulnerabilities majacQ/blueprint#204

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new class for DateRangePicker state internals #619

Create new class for DateRangePicker state internals #619

leebyp commented Feb 4, 2017

leebyp commented Feb 7, 2017

cmslewis Feb 8, 2017

cmslewis Feb 8, 2017

leebyp Feb 9, 2017

cmslewis Feb 8, 2017

giladgray Feb 8, 2017

cmslewis Feb 8, 2017

giladgray Feb 8, 2017

cmslewis Feb 8, 2017

giladgray Feb 8, 2017 •

edited

Loading

leebyp Feb 9, 2017

giladgray Feb 8, 2017

giladgray Feb 8, 2017

giladgray Feb 8, 2017

giladgray Feb 8, 2017

giladgray Feb 8, 2017

leebyp Feb 9, 2017

giladgray Feb 8, 2017

leebyp Feb 9, 2017

giladgray Feb 8, 2017

giladgray Feb 8, 2017

giladgray Feb 8, 2017

leebyp Feb 9, 2017

blueprint-bot commented Feb 9, 2017

blueprint-bot commented Feb 9, 2017

giladgray Feb 10, 2017 •

edited

Loading

leebyp Feb 10, 2017 •

edited

Loading

giladgray Feb 10, 2017

giladgray Feb 10, 2017

leebyp Feb 10, 2017

blueprint-bot commented Feb 10, 2017

blueprint-bot commented Feb 10, 2017

blueprint-bot commented Feb 11, 2017


		type MonthAndYear = [number, number];

		function getPreviousMonth([month, year]: MonthAndYear): MonthAndYear {


		import { Months } from "./months";

		export class DisplayMonth {

Create new class for DateRangePicker state internals #619

Create new class for DateRangePicker state internals #619

Conversation

leebyp commented Feb 4, 2017

leebyp commented Feb 7, 2017

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

giladgray Feb 8, 2017 • edited Loading

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

blueprint-bot commented Feb 9, 2017

rename DisplayMonth to MonthAndYear

blueprint-bot commented Feb 9, 2017

switch displaymonth comparator to conventional return values

giladgray Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

leebyp Feb 10, 2017 • edited Loading

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

blueprint-bot commented Feb 10, 2017

change DateRangePicker internal state name

blueprint-bot commented Feb 10, 2017

simplify compare month and year

blueprint-bot commented Feb 11, 2017

fix utils function

giladgray Feb 8, 2017 •

edited

Loading

giladgray Feb 10, 2017 •

edited

Loading

leebyp Feb 10, 2017 •

edited

Loading