Respect showOutsideDays prop for single month date range #6862

evansjohnson · 2024-06-24T20:47:25Z

Fixes #4373

Checklist

Includes tests
Update documentation

Changes proposed in this pull request:

When a DateRangePicker3 is limited to a single month, respect the showOutsideDays passed on dayPickerProps. Per https://github.com/palantir/blueprint/pull/586/files#r98813760 it was explicitly decided that showing outside days when multiple months are shown is not intended, and I agree that it still leads to some weird questions, but for a single month these issues do not exist.

Reviewers should focus on:

Is this the behavior we want?
Do the updated docs look good?
Only fixing on DateRangePicker3 since DateRangePicker is deprecated and frozen.

Alternate option

Simply document that showOutsideDays is never respected for DateRangePicker3

Screenshot

evansjohnson · 2024-06-27T15:57:57Z

packages/datetime2/test/components/dateRangePicker3Tests.tsx

+
+ assert.equal(left.findOutsideDays().first().getDOMNode().computedStyleMap().get("pointer-events")?.toString(), "none");
+
+ // simulated clicks appear to not respect pointerEvents style or else would test with below


no activity on it but I found enzymejs/enzyme#2340

open to suggestions for better ways to disable the interaction or better ways to test this

evansjohnson · 2024-06-27T15:58:17Z

packages/datetime2/test/components/dateRangePicker3Tests.tsx

+
+ it("allows outside days to be shown for single month date range", () => {
+ const { left } = render({ initialMonth: new Date(2015, Months.DECEMBER, 2), singleMonthOnly: true, dayPickerProps: { showOutsideDays: true } });
+ assert.equal(left.findOutsideDays().first().getDOMNode().computedStyleMap().get("visibility")?.toString(), "visible");


open to suggestion for better way to test visibility here

evansjohnson · 2024-06-27T15:59:24Z

packages/datetime2/test/components/dateRangePicker3Tests.tsx

@@ -1332,6 +1332,33 @@ describe("<DateRangePicker3>", () => {
 });
 });

+ describe("outside days", () => {


these tests are very closely tied to the implementation but figured it was good to get something in testing the behavior so this doesn't unintentionally regress

evansjohnson · 2024-06-27T16:04:43Z

packages/datetime2/src/components/date-range-picker3/_date-range-picker3.scss

+
+ /* stylelint-disable max-line-length */
+ &.#{$ns}-datepicker.#{$ns}-daterangepicker-single-month.#{$ns}-daterangepicker-showing-outside-days .rdp-day_outside {
+ pointer-events: none; // weird interactions happen if allowing outside days to be clicked


Note that without this, selecting a date in the earlier "outside days" switches to the previous month. This doesn't seem ideal since there will not be many days ahead of that initial date selected once we switch to the previous month, but the user can reselect the month after making the start date selection.

Also note that we only need to worry about click and not keyboard nav events here because we still want to allow keyboard nav with arrow keys to navigate between months. The difference is that keyboard nav is navigation, then selection, while mouse interaction is selection and navigation which felt a bit jarring.

Open to suggestions for a better solution here.

I would be curious for @alex-dales's opinion. Playing around with the UI, I actually found it surprising that I couldn't click on the grayed out dates. It might not be obvious to some users that they have to switch to the previous month intentionally to select these "outside day" dates?

svc-palantir-github · 2024-06-27T20:31:44Z

assert warn

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

gluxon

Implementation and tests look good! (Or, the tests look as good as they can be. I don't know of a better way to check for visibility. 😂)

I lean towards allowing the outside days to be click-able, but will let you and @alex-dales make the final call on the right UX for that.

gluxon · 2024-08-06T21:32:32Z

packages/datetime2/src/components/date-range-picker3/_date-range-picker3.scss

@@ -59,6 +59,14 @@
 }
 }

+
+ /* stylelint-disable max-line-length */


nitpick: I usually like to use stylelint-disable-next-line whenever possible to avoid the need to re-enable the lint rule further down.

Suggested change

/* stylelint-disable max-line-length */

// stylelint-disable-next-line max-line-length

gluxon · 2024-08-06T21:32:52Z

packages/datetime2/src/components/date-range-picker3/_date-range-picker3.scss

+ pointer-events: none; // weird interactions happen if allowing outside days to be clicked
+ visibility: visible; // showing outside days is allowed for single months
+ }
+ /* stylelint-enable max-line-length */


nitpick: It should be possible to remove this after using // stylelint-disable-next-line above.

gluxon · 2024-08-06T21:44:33Z

packages/datetime2/test/components/dateRangePicker3Tests.tsx

@@ -1332,6 +1332,55 @@ describe("<DateRangePicker3>", () => {
 });
 });

+ describe("outside days", () => {
+ it("visually hides outside days when contiguous months are shown, even if showOutsideDays is true", () => {
+ const warnSpy = stub(console, "warn");


nitpick: Might be useful to clean up this stub at the end of this function with:

warnSpy.restore();

Otherwise this console.warn stub stucks around for all tests that run after this one.

gluxon · 2024-08-06T22:07:37Z

packages/datetime2/src/components/date-range-picker3/_date-range-picker3.scss

+
+ /* stylelint-disable max-line-length */
+ &.#{$ns}-datepicker.#{$ns}-daterangepicker-single-month.#{$ns}-daterangepicker-showing-outside-days .rdp-day_outside {
+ pointer-events: none; // weird interactions happen if allowing outside days to be clicked


I would be curious for @alex-dales's opinion. Playing around with the UI, I actually found it surprising that I couldn't click on the grayed out dates. It might not be obvious to some users that they have to switch to the previous month intentionally to select these "outside day" dates?

gluxon · 2024-08-06T22:09:37Z

packages/docs-app/src/examples/datetime2-examples/dateRangePicker3Example.tsx

+ <Tooltip content="Only respected when showing a single month calendar">
+ <Switch
+ checked={this.state.showOutsideDays}
+ className="keep-bottom-margin"


super nitpick: It looks like the bottom margin here is getting reset back to 0 by some other CSS selector.

svc-palantir-github · 2024-08-06T22:26:11Z

assert warn

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson added 7 commits June 24, 2024 16:02

Respect showOutsideDays prop for single month date range

0a156c1

does this work?

4ec0b97

format

5ef0a98

lint

2ed584c

keep it simple

8b74c8b

add warning for prop combo

5a2b3a7

format

f55b73c

evansjohnson added the Do not review label Jun 25, 2024

disable pointer events

e4e88e5

evansjohnson removed the Do not review label Jun 26, 2024

evansjohnson added 9 commits June 26, 2024 17:32

break up selector

62b901c

fix build

93f551b

docs

592f938

more fix

d7f7021

docs

f412b17

naming

67d95bc

format

9856095

tests

2cfa0c0

take out .only

51512a9

evansjohnson commented Jun 27, 2024

View reviewed changes

evansjohnson added the no changelog label Jun 27, 2024

evansjohnson added 2 commits June 27, 2024 12:00

typo

e894b59

comment on same line

018fae3

palantir deleted a comment from svc-palantir-github Jun 27, 2024

evansjohnson commented Jun 27, 2024

View reviewed changes

format

c4a82a8

palantir deleted a comment from svc-palantir-github Jun 27, 2024

reenable

b8f0ec3

evansjohnson added 2 commits June 27, 2024 15:47

lint

1121b81

lint

aec0e5d

palantir deleted a comment from svc-palantir-github Jun 27, 2024

assert warn

fd37b66

evansjohnson marked this pull request as ready for review June 27, 2024 20:32

evansjohnson assigned gluxon Jun 27, 2024

gluxon approved these changes Aug 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect showOutsideDays prop for single month date range #6862

Respect showOutsideDays prop for single month date range #6862

evansjohnson commented Jun 24, 2024 •

edited

Loading

evansjohnson Jun 27, 2024

evansjohnson Jun 27, 2024

evansjohnson Jun 27, 2024

evansjohnson Jun 27, 2024

gluxon Aug 6, 2024

svc-palantir-github commented Jun 27, 2024

gluxon left a comment

gluxon Aug 6, 2024

gluxon Aug 6, 2024

gluxon Aug 6, 2024

gluxon Aug 6, 2024

gluxon Aug 6, 2024

svc-palantir-github commented Aug 6, 2024


		assert.equal(left.findOutsideDays().first().getDOMNode().computedStyleMap().get("pointer-events")?.toString(), "none");

		// simulated clicks appear to not respect pointerEvents style or else would test with below

	/* stylelint-disable max-line-length */
	// stylelint-disable-next-line max-line-length

Respect showOutsideDays prop for single month date range #6862

Are you sure you want to change the base?

Respect showOutsideDays prop for single month date range #6862

Conversation

evansjohnson commented Jun 24, 2024 • edited Loading

Fixes #4373

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Alternate option

Screenshot

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

svc-palantir-github commented Jun 27, 2024

assert warn

gluxon 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

svc-palantir-github commented Aug 6, 2024

assert warn

evansjohnson commented Jun 24, 2024 •

edited

Loading