Skip to content
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

Feature/2824 date input #2864

Open
wants to merge 22 commits into
base: next
Choose a base branch
from
Open

Feature/2824 date input #2864

wants to merge 22 commits into from

Conversation

kwongz
Copy link
Contributor

@kwongz kwongz commented Nov 28, 2024

Description

Created DateInput component that builds off the DateRange structure. Redirects /components/date-range/ --> /components/date-input/

Allows users to access a single date or ranged date input calendar

image

image

Checklist

  • For UI or styling changes, I have added a screenshot or gif showing before & after
  • I have added a changeset
  • I have added to the docs where applicable
  • I have added to the VS Code extension where applicable

@kwongz
Copy link
Contributor Author

kwongz commented Nov 29, 2024

fixes #2824

@kwongz kwongz requested a review from archiewood November 29, 2024 15:49
data={orders_by_day}
dates=day
title="Select a Date Input"
range
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this example uses a range, but is in the wrong section? remove?

Suggested change
range

select
*
from ${orders_by_day}
where day = '${inputs.range_filtering_a_query.value}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example chart looks odd, A barchart with a single day is rarely used.

I think you'd be using this as a one sided filter.

Suggested change
where day = '${inputs.range_filtering_a_query.value}'
where day > '${inputs.range_filtering_a_query.value}'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice yeah, i like this better

/>
<PropListing
name="title"
options="string"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks odd to just have a floating number in a button. not obvious what to do without a title?

Maybe we can make this title a default?

I know @mcrascal has an different view so happy to discuss

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's definitely missing something, and easily overlooked on its own

@@ -0,0 +1,302 @@
---
title: Date Input
sidebar_position: 1
Copy link
Member

@archiewood archiewood Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor point, we should have a description: - we should be doing this for all components going forward

- orders_by_day.sql
---

Creates a date picker that can be used to filter a query.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Creates a date picker that can be used to filter a query.
A date input component allows the user to select a date or a range of dates. The selected dates can be used as inputs to queries or components.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be the description also

@kwongz
Copy link
Contributor Author

kwongz commented Nov 29, 2024

@archiewood @mcrascal

Date Input w/ calendar (@steeze-ui/svelte-icon)

image

Date Input w/ inline title

image

Date Input w/ range + title (no calendar icon)

image

@kwongz
Copy link
Contributor Author

kwongz commented Dec 2, 2024

Added more margin between icon and text (m1 --> m2).

image

Date range for reference

image

@archiewood
Copy link
Member

archiewood commented Dec 2, 2024

calendar icon is improved

The separating bar between the Date Input and Jan 1, 1970 looks different to that used in the Dropdown to me (too heavy, does not extend high or low enough)

Can we match the styling

@kwongz
Copy link
Contributor Author

kwongz commented Dec 2, 2024

lowered the stroke and change text coloring

image

@kwongz
Copy link
Contributor Author

kwongz commented Dec 4, 2024

Here are some options we can fix with custom stroke width, and perhaps exploring other icons. I think

I'm noticing we can fix some pixelation with a lower stroke width when the text is black.

Calendar stroke-[1.8px] text-black

image

stroke-[2px]

image

stroke-[2.1px]

image

stroke-[2.2px]

image

Calendar Down stroke-[2px]

image

Calendar Event stroke-[2px]

image

@archiewood
Copy link
Member

I think maybe stroke-[1.8px] text-gray-700 with Calendar Event might look good?

@archiewood
Copy link
Member

although I actually like what is currently in the deploy preview

@kwongz
Copy link
Contributor Author

kwongz commented Dec 5, 2024

@archiewood
I just uploaded a preview of the calendar event, i think they both look good, I'm leaning towards Calendar Event now haha

image

image

@archiewood
Copy link
Member

Calendar event looks great!

Copy link
Member

@archiewood archiewood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs engineering code review from as the changes are quite surgical.

@kwongz kwongz requested a review from ItsMeBrianD December 5, 2024 16:22
@kwongz
Copy link
Contributor Author

kwongz commented Dec 5, 2024

@ItsMeBrianD

Moved some functions used both in DateRange and DateInput, into a helper.js
Removed _DateRange (not being used)

I tried moving some of the query-building logic/ input store, but it felt like my changes were making the code more complicated, so I left them in the respective components.

@csjh csjh self-requested a review December 11, 2024 19:44
export let data;
/** @type {string | undefined} */
export let dates;
/** @type {[]string | undefined} */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @type {[]string | undefined} */
/** @type {string[] | undefined} */

/** @type {string} */
export let name;
/** @type {string | undefined} */
export let title;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional props like these should have a default value of undefined, otherwise a bunch of warnings pop up in dev

if (range) {
$inputs[name] = { value: undefined, start: startString, end: endString };
} else {
$inputs[name] = { value: startString, start: startString, end: undefined };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set start here? Seems more like it should just be start/end for range vs. value for non-range

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'd prefer to omit the undefined props rather than giving them the value of undefined

Copy link
Member

@archiewood archiewood Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my suggestion

My initial thought was start is probably worth keeping start as someone might change from <DateInput range=true to range=false, and then it break the rest of their code.

honestly, probably should remove start from DateInput range=false

now I think about it you are likely going to have to change the SQL anyway from between to =

selectedPreset = targetPreset;
if (range) {
selectedDateInput = targetPreset.range ?? targetPreset.value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but the targetPreset.value seems like dead code - doesn't seem like we allow .value in elements of presets

{:else if selectedDateInput && !range}
{#if title}
{title}
<Separator oritentation="vertical" class="mx-2 h-4 w-[1px]" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Separator oritentation="vertical" class="mx-2 h-4 w-[1px]" />
<Separator orientation="vertical" class="mx-2 h-4 w-[1px]" />


$: range = toBoolean(range);

const exec = getQueryFunction();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ItsMeBrianD is there an alternative we should prefer to direct Query.create/getQueryFunction here


// Mock "today"
import MockDate from 'mockdate';
MockDate.set('2024-12-30');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this persist after you switch to another story?

Would probably be good to reset it

Suggested change
MockDate.set('2024-12-30');
MockDate.set('2024-12-30');
onMount(() => () => MockDate.reset());

}
}

let currentDate = dateToYYYYMMDD(new Date(Date.now()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let currentDate = dateToYYYYMMDD(new Date(Date.now()));
let currentDate = dateToYYYYMMDD(new Date());

new Date() will create a date with the current date/time, no need for Date.now()

@@ -39,6 +36,9 @@
/** @type {string | undefined} */
export let defaultValue;

/** @type {boolean} */
let range = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a range prop on DateRange? DateRange should always be a range in my opinion

/** @type {string | undefined} */
export let defaultValue;
/** @type {boolean} */
export let range = false;
Copy link
Member

@archiewood archiewood Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any user facing boolean should accept a string also

<DateInput
  range=true
/>

as well as

<DateInput
  range={true}
/>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants