Skip to content

Conversation

@Jialecl
Copy link
Collaborator

@Jialecl Jialecl commented Nov 3, 2022

Checklist
(Check off all the items before submitting)

  • Build process is done without errors and all tests pass in /lib directory.
  • Self-reviewed the code prior to submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Purpose
Material-ui datepicker removed and added month picker to the new calendar

Additional context
Dayjs has been used to format and help to calculate the correct date within the calendar while navigating.

Closes #1368 #855

@Jialecl Jialecl marked this pull request as ready for review November 3, 2022 13:42
@aidamag aidamag self-assigned this Nov 3, 2022
@aidamag aidamag removed their assignment Nov 4, 2022
Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

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

More comments:

  • Date picker seems to overlap the text input.

image

  • Each day's font-size is smaller than in the previous date picker.
  • Focus behaviour is not working as expected. The Tab should move the focus throw the focusable elements inside the calendar dialog. These components are:
    • Year button.
    • Month button.
    • Previous month button.
    • Next month button.
    • Only the selected day. Right now, when you tab, the focus goes throw every day of the month, which is a bit tedious and not correct. Only the selected (active) day should be focusable. See this or any other calendar.

Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

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

Summary:

  • The date picker is been pushed by the error message.

image

  • Depending on the month and its string length, the right arrow (next month) changes its positioning.

The focus is not always working as expected:

  • If you open the calendar and then click the date input, the date picker does not close.
  • If you close the date picker with the Esc key, the focus doesn't go back to the date input, as it was in the previous one or the one from W3C.
  • If you open the calendar, then click on a space (for example, the blank spaces on the right side of the selected month/year) and press any arrow button, the dialogue gains the focus:

image

@Jialecl Jialecl marked this pull request as draft November 16, 2022 10:04
@aidamag
Copy link
Contributor

aidamag commented Dec 13, 2022

Width of the calendar is different comparing it with the designs:

image

image

There is more space in the sides than in the design:
image

  • Add box-sizing: border-box; in the DatePicker div.
  • Change the width of YearPickerContainer to 252px.

Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

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

Review

General

  • I think we should update the documentation in another PR. There are still some missing updates we need to do (images, descriptions, etc) and the PR could become too big.

Date Picker

  1. If the focus leaves the window, the date picker does not close.
  2. Why does the outline from the previous/next month's arrows have an offset of -2px? It doesn't make sense since it has enough space to appear. The same question goes for the year dropdown button.
  3. The box-shadow was designed to be complemented with a border (like in the Select) because on certain screens it can seem not visible:

image

  1. Sometimes, the focus leaves a kind of visual trace:

image

I think this is caused by the component dimensions plus the styling itself, maybe the outline offset and the border set to none are provoking this.

Year selector

  1. Inconsistent scroll: Quick arrow presses can make the scroll work a bit strange and not always centre the visually focused year.
  2. The focus has some sort of memory, which I think doesn't quite fit the intended functionality. I mean, if the focus goes out from the years' list and comes back, it goes to the last focused item.

Calendar

  1. Focus proposal for the Calendar (currently, is not working as expected):

    • If there's a day selected for the current month, the focus goes to this one.
    • If there's no day selected but the actual date and the actual month are being displayed, the focus goes to the actual day.
    • If both previous conditions are not true, the focus goes to the first day of the month.
  2. If the day selected is from the previous/next month and it is also displayed in the current view (the light grey color days), it must be displayed as selected. See for example the calendar from Cloudscape

image

  1. The days are too close to each other horizontally, but they have a small amount of space vertically. I think both rows and columns should have the same gap.

Date Picker (1)

Date Picker (2)

  1. Same issue as in the year selector: The focus has some sort of memory, which I don't fully agree with. It makes it a bit unpredictable for the user to know where the focus is going to be placed since sometimes it's memorized and other times not.

Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

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

Some comments:

  • If you change the year, the current day appears focused instead of the first one.
  • If you click on a day and then press the tab button, the calendar closes instead of going to the previous month's arrow.
  • The scroll does not behave well yet. Same thoughts as in the last review and adding to that, we should remove the smooth behaviour of it, as other scrollable components are being used.
  • Tokens need to be updated based on the designer's choices and opinions. We cannot make any changes by ourselves.
  • Storybooks:
    • We need to check every state and from the buttons of the Year Picker and the Calendar (focus, hover, active, etc.)
    • We need to check that the date picker appears over other components with a z-index higher than 0 (check the dropdown example to get an idea).

Copy link
Collaborator

@marcialfps marcialfps left a comment

Choose a reason for hiding this comment

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

💯

@GomezIvann
Copy link
Collaborator

This branch is gonna be merged but there is still some work to be done.

@GomezIvann GomezIvann merged commit 2750716 into master Dec 23, 2022
@GomezIvann GomezIvann deleted the jialecl-calendar branch December 23, 2022 09:13
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.

Remove Material UI dependencies from Date Input

5 participants