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

feat(input-date-picker, date-picker): improve date picking experience #8402

Merged
merged 197 commits into from
Oct 23, 2024

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Dec 12, 2023

Related Issue: #3455

Summary

Update calcite-date-picker & calcite-input-date-picker UI & UX.

4D1CFC3C-8FF9-4493-9178-4DEDA0417031

Key changes

  • display two calendars for range irrespective of layout.
  • No longer switches focus from day to end input when startDate is selected initially.
  • Month selection is possible via select menu
  • No longer positions the date-picker component relative to endInput when endInput is focused in range.
  • Dates from previous months are not visible in range calendar.
  • Divider indicator icons are removed in horizontal layout for range in input-date-picker.
  • No longer uses chevron icon which indicate the open status of input-date-picker in startInput field.

Other issues resolved :
#6321
#6410
#10301
#10291
#10113
#10243
#10490
#10069

Blocked issues: #9167

Wiki https://github.com/Esri/calcite-design-system/wiki/date%E2%80%90picker-enhancement-%238402

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Dec 12, 2023
@macandcheese
Copy link
Contributor

Is the intention that these are separate public components? Or all housed within one public component?

I know the native input can be type="month", but from my understanding, across other design systems, this approach seems (more?) common: https://mui.com/x/react-date-pickers/date-picker/#views

@anveshmekala
Copy link
Contributor Author

anveshmekala commented Dec 12, 2023

Is the intention that these are separate public components? Or all housed within one public component?

I know the native input can be type="month", but from my understanding, across other design systems, this approach seems (more?) common: https://mui.com/x/react-date-pickers/date-picker/#views

Yeah, the idea is to have independent input-month-picker and input-date-picker components which makes it easier to maintain. Started off with month-picker though which can be used in input-month-picker and also integrated to current date-picker to allow user change month from a dropdown style.

Concurrently, I am testing a different approach to use combobox for input-month-picker and input-year-picker (similar to input-time-zone) which seems promising but wouldn't work if we need to display two scroll bars one for month and one for year.

I also dig the idea mentioned in the issue for input-month-picker which is more like a table with year at the top. It doesn't require two scroll bars.

@macandcheese
Copy link
Contributor

macandcheese commented Dec 12, 2023

Concurrently, I am testing a different approach to use combobox for input-month-picker and input-year-picker (similar to input-time-zone) which seems promising but wouldn't work if we need to display two scroll bars one for month and one for year.

IMO, this approach is just an implementation of a Combobox, a user can already set something like that up. I'd think ours would be more custom picking experiences like the examples linked in the original issue / Material example (however the components are structured).

I also dig the idea mentioned in the issue for input-month-picker which is more like a table with year at the top. It doesn't require two scroll bars.

Agreed, I think that's more appropriate for a component offering. If it's just a Combobox, we're only setting up an existing component with a set of pre-configured options, not providing a unique selection experience.

@anveshmekala anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 1, 2024
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Aside from a few comments, I think this is good to go! Awesome job, @anveshmekala! 😎

📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅
📅👈📅📅📅👈📅📅👈👈📅📅👈👈👈📅📅👈👈👈📅👈👈👈👈📅👈📅
📅👈👈📅📅👈📅👈📅📅👈📅📅👈📅📅👈📅📅📅📅👈📅📅📅📅👈📅
📅👈📅👈📅👈📅👈📅📅👈📅📅👈📅📅👈📅📅📅📅👈👈👈📅📅👈📅
📅👈📅📅👈👈📅👈📅📅👈📅📅👈📅📅👈📅📅📅📅👈📅📅📅📅📅📅
📅👈📅📅📅👈📅📅👈👈📅📅👈👈👈📅📅👈👈👈📅👈👈👈👈📅👈📅
📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅📅

@@ -53,23 +53,47 @@ export default {
};

export const simple = (args: InputDatePickerStoryArgs): string => html`
<div style="width: 400px">
<style>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: not critical for this PR, but you might be able to simplify setting custom width and height by using a custom decorator (example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will create a followup PR.


async function selectDayInMonthById(id: string, page: E2EPage): Promise<void> {
const day = await page.find(
`calcite-date-picker>>> calcite-date-picker-month >>> calcite-date-picker-day[current-month][id="${id}"]`,
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: can you add a space before the first piercing selector?

);
expect(firstDayInNextMonth.classList.contains("current-day")).toBe(false);
currentDay = await page.find(
"calcite-date-picker >>> calcite-date-picker-month >>> calcite-date-picker-day.current-day",
Copy link
Member

Choose a reason for hiding this comment

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

Can you create a follow-up issue to see if we can make these tests more robust? The complex selectors might make them a bit brittle.

await page.keyboard.press("Enter");
await page.waitForChanges();
expect(await calendar.isVisible()).toBe(false);
expect((await inputDatePicker.getProperty("value"))[1]).toEqual("2024-07-02");
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: splitting up the value (storing in a var) and assertion might help with readability.

Copy link
Contributor Author

@anveshmekala anveshmekala Oct 2, 2024

Choose a reason for hiding this comment

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

Is the below syntax suggested?

    const [, endDate] = await inputDatePicker.getProperty("value");
    expect(endValue).toEqual("2024-07-02");


expect(await date.getAttribute("aria-label")).toBe("Year");
const yearInput = await page.find(`calcite-date-picker-month-header >>> input`);
expect(yearInput.getAttribute("aria-label")).toBe("Year");
Copy link
Member

Choose a reason for hiding this comment

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

Nice. So we could do something like the following, right?

expect(yearInput.getAttribute("aria-label")).toBe(messages.year);

if (!Array.isArray(valueAsDate) && valueAsDate && valueAsDate !== this.activeDate) {
this.activeDate = new Date(valueAsDate);
}

Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: For readability, can you restore these newlines? It should help scanning the code since there's a bit of if statements here.

@@ -70,9 +84,21 @@ export class DatePickerMonthHeader {
* @internal
* @readonly
*/
// eslint-disable-next-line @stencil-community/strict-mutable -- updated by t9n module
// eslint-disable-next-line @stencil-community/strict-mutable
@Prop({ mutable: true }) messages: DatePickerMessages;
Copy link
Member

Choose a reason for hiding this comment

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

How so? mutable is for internal prop updates and I don't see where it might get updated internally.

private setSelectMenuWidth(select: HTMLCalciteSelectElement): void {
const selectEl = select.shadowRoot.querySelector("select");
let selectedOptionWidth: number;
const fontStyle = getComputedStyle(selectEl).font;
Copy link
Member

Choose a reason for hiding this comment

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

We could add inherit here & reference the font css prop at :host.

I dig it!


private handleKeyDown = (event: KeyboardEvent): void => {
if (event.key === "ArrowDown" || event.key === "ArrowUp") {
event.stopPropagation();
Copy link
Member

@jcfranco jcfranco Oct 2, 2024

Choose a reason for hiding this comment

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

We cancel the event for these cases. input-date-picker will ignore canceled arrow key presses.

Actually, after taking a closer look, this is a totally different issue. We can address it separately.

}

private async setSelectMenuIconOffset(select: HTMLCalciteSelectElement): Promise<void> {
const iconEl = select.shadowRoot.querySelector("calcite-icon");
Copy link
Member

Choose a reason for hiding this comment

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

Could we get the icon container dimensions from the design spec and tokens? If not, I’m inclined to hardcode the sizes (based on the spec) while we explore alternatives in a follow-up issue.

@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 2, 2024
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 3, 2024
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 9, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 19, 2024
@anveshmekala anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Oct 23, 2024
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 23, 2024
@anveshmekala anveshmekala merged commit 24d43ad into dev Oct 23, 2024
13 checks passed
@anveshmekala anveshmekala deleted the anveshmekala/3455-add-month-and-year-picker branch October 23, 2024 18:12
@github-actions github-actions bot added this to the 2024-10-29 - Oct Milestone milestone Oct 23, 2024
benelan added a commit that referenced this pull request Oct 25, 2024
…orepo

* origin/dev:
  build(deps): update typescript-eslint monorepo to v7.18.0 (#10593)
  feat(tile): add design tokens (#10476)
  deprecate: deprecate web-adaptor in favor of web-adapter (#10598)
  docs: update component READMEs (#10600)
  build(deps): update dependency @rollup/plugin-replace to v6 (#10604)
  chore: release main (#10597)
  ci(chromatic): build storybook even when skipping snapshots (#10589)
  build(deps): update dependency axe-core to v4.10.1 (#10566)
  ci: add calcite-ui-icons label to relevant package PRs (#10590)
  chore: release next
  feat: add buffer point, buffer polygon, buffer polyline, contour, offset (#10594)
  feat(input-date-picker, date-picker): improve date picking experience (#8402)
  chore: release next
  fix: updated web-adapter name (#10581)
  build(deps): update nrwl monorepo to v19.8.6 (#10575)
  build(deps): update dependency tsx to v4.19.1 (#10574)
  build(deps): update dependency @cspell/eslint-plugin to v8.15.4 (#10565)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing. visual changes Issues with visual changes that are added for consistency, but are not backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants