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

[vNext] Redesing to material-ui 2.0 #1416

Merged
merged 53 commits into from
Dec 23, 2019
Merged

[vNext] Redesing to material-ui 2.0 #1416

merged 53 commits into from
Dec 23, 2019

Conversation

dmtrKovalenko
Copy link
Member

@dmtrKovalenko dmtrKovalenko commented Dec 5, 2019

This PR is a part of v4 development (#1293)

Description

  • Common design changes like toolbar, dimensions, paddings, etc...
  • Redesign datepicker with toolbar
  • Change moving views logic to be inside the calendar and change year with triangle icon
  • Fix tests <-- In progress right now, moving our tests from enzyme to cypress components tests
  • Redesign timepicker
  • Redesign datetime picker

Note that new layout for mobile/desktop variant will be in the next PR, here just changing everything inside the components.

This PR hardly in development right now, but please @oliviertassinari take a brief look of the deployment preview for how it looks and feels

@cypress
Copy link

cypress bot commented Dec 6, 2019



Test summary

50 0 0 0


Run details

Project material-ui-pickers
Status Passed
Commit 2bb51f3
Started Dec 23, 2019 11:59 AM
Ended Dec 23, 2019 12:00 PM
Duration 01:15 💡
OS Linux Debian - 8.11
Browser Electron 61

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

lib/src/DatePicker/DatePickerToolbar.tsx Outdated Show resolved Hide resolved
lib/src/DatePicker/DatePickerToolbar.tsx Outdated Show resolved Hide resolved
lib/src/_shared/icons/PenIcon.tsx Outdated Show resolved Hide resolved
lib/src/views/Calendar/Calendar.tsx Outdated Show resolved Hide resolved
lib/src/views/Calendar/CalendarHeader.tsx Outdated Show resolved Hide resolved
@@ -127,15 +181,11 @@ CalendarHeader.displayName = 'CalendarHeader';
CalendarHeader.propTypes = {
leftArrowIcon: PropTypes.node,
rightArrowIcon: PropTypes.node,
disablePrevMonth: PropTypes.bool,
disableNextMonth: PropTypes.bool,
};

CalendarHeader.defaultProps = {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered to use the prop spread default values instead of default props?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is primarily used for typescript typings. This is just so old code, will move it. (But there is a problem with auto generating default props values)

Copy link
Member

Choose a reason for hiding this comment

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

We might have a solution for this. The default props generation for API purpose was solve in the mui-org/material-ui repository. @eps1lon took care of it.
It's a good reminder that we need to look into how we can extract the API from TypeScript sources, and not only JavaScript ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now I created custom script that reads jsdoc tags on props like @default "someValue" and primarily use it and getting rid from default props.

Copy link
Member

Choose a reason for hiding this comment

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

Our handler for react-docgen and default props via default values in object rest spread can be found in https://github.com/mui-org/material-ui/blob/6f7318cdd5c1c2887860c0d56212a3e42d107082/docs/src/modules/utils/defaultPropsHandler.js#L8

Copy link
Member Author

Choose a reason for hiding this comment

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

@eps1lon don't you use react-docgen-typescript?

lib/src/views/Calendar/CalendarView.tsx Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to staging December 17, 2019 15:58 Inactive
@dmtrKovalenko
Copy link
Member Author

dmtrKovalenko commented Dec 20, 2019

  1. Yeah I think we can remove this prop. It looks useless.
  2. Related to the years. They are generating from the minDate to maxDate. By default the range is 1900-01-01 to 2100-01-01.
  3. Yes, it looks like a problem, but still, you can switch to year view from toolbar and also (needs to be implemented) from calendar header
  4. Will try to use ButtonBase and check is it reduces performance

(haha I am seeing that keyboard navigation is stopping after the animation, I suppose the problem is losing focus. Looks like we need a global useKeyDown)

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 20, 2019

  1. Thanks for the context. I thought the date picker had no limits. What happens if we change the props to remove the min and max date limits? I have tried <DatePicker minDate={null} maxDate={null} but it didn't seem to have an impact. https://blueprintjs.com/docs/#datetime/datepicker behaves like the current implementation. (Oh, I see that blueprints went with http://react-day-picker.js.org/, it sounds like its a good signal that we can benchmark with it :)).
    What do you think of removing the default limits, and having ±100 years sliding window around the "active" date?

Unrelated, we might have an opportunity to improve a11y. I was wondering about the usage of grid roles for the day grid: https://www.w3.org/TR/wai-aria-practices/#table.

  1. I had a closer look at the "calendar" implementation. Do we need the <div role="presentation"> around the cells? A lighter DOM node could help with performance.

@dmtrKovalenko
Copy link
Member Author

dmtrKovalenko commented Dec 22, 2019

Addressed some feedback:

  1. Moving to button base saved 2ms of rendering on my machine (from 10ms to 8ms of rendering dialog with transition group)
  2. My concern about the dynamic range by default - will it be obvious for developers? Right now it is clear that minDate will not render anything below. Moreover, it will require additional calculations on the rendering step (honestly it should not be a problem)
  3. Yes, we need this wrapper with role="presentation". The main reason is that the day component is overridable, so we need a way to be sure that all event handlers are attached even when the component is overridden.
  4. Keyboard animation was broken by moving to the relative keydown listeners. Right now fixed and will not freeze after switching to the next month.
  5. https://axesslab.com/accessible-datepickers/ I do think that primary way of input day should be a keyboard. I suppose that's why google added an input button even into the mobile datepicker view. I am not sure how users can properly understand the selected date from the calendar day list. I am planning also to significantly improve accessibility by adding proper aria-labels to the date values and properly handle keyboard input. But also it needs to be done after keyboard input from mobile and to be a separate PR

@oliviertassinari
Copy link
Member

  1. I have tried the following diff, to benchmark a month switch in dev mode
diff --git a/lib/src/views/Calendar/Day.tsx b/lib/src/views/Calendar/Day.tsx
index 100cad6..d28a1e8 100644
--- a/lib/src/views/Calendar/Day.tsx
+++ b/lib/src/views/Calendar/Day.tsx
@@ -70,9 +70,9 @@ export const Day: React.FC<DayProps> = ({
   });

   return (
-    <IconButton className={className} tabIndex={hidden || disabled ? -1 : 0} {...other}>
-      <Typography variant="body2" color="inherit" children={children} />
-    </IconButton>
+    <span>
+      {children}
+    </span>
   );
 };

before
Capture d’écran 2019-12-22 à 18 56 49

after
Capture d’écran 2019-12-22 à 18 56 41
2. My main concern would be about: if somebody needs to significantly increase the min/max range. How long does it take to render the whole year list (>1s?) and how much to scroll to find the value the user needs?
3. For this problem, we frequently ask for developers to spread the props to the root element, it could be an alternative. I have tried without the wrapping div, it doesn't seem to have a significant impact as we have in 1. It seems that we can ignore performance on the mater.
4. Nice
5. +1 for keyboard and a11y exploration

@dmtrKovalenko
Copy link
Member Author

dmtrKovalenko commented Dec 23, 2019

  1. I am not seeing such significant performance impact with ButtonBase + <span> instead of IconButton + Typography. But still removing it in favor of simplicity and fewer dom nodes
  2. Here is a snapshot switching to year selection with rendering a list from 1700 to 2300. Pretty fast, and thanks to animation it is even more smooth.
    image

Also, I am planning to render 4 years in the column for desktop pickers. For mobile it is too much cause touch targets will be too small.

From material.io: Mobile calendar pickers. They aren’t ideal for selecting dates in the distant past or future that require more navigation, such as entering a birth date or expiration date.

  1. Haha, I remember why we need an additional day wrapper :)
<DatePicker
  // We are passing already rendered day node right to the override function. Super usefull for adjustign rendering
  renderDay={(day, selectedDate, isInCurrentMonth, dayComponent) => {
     const isSelected = isInCurrentMonth && selectedDays.includes(date.getDate());

     return <Badge badgeContent={isSelected ? '🌚' : undefined}>{dayComponent}</Badge>;
   }}
 />
  1. Will move this work to separate PR

@dmtrKovalenko
Copy link
Member Author

I think this should be ready to go? Moving to some work in date-io 💪

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

One step at the time 👍

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