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

[DatePicker] Update to match the specification (desktop) #1648

Closed
oliviertassinari opened this issue Apr 12, 2020 · 5 comments
Closed

[DatePicker] Update to match the specification (desktop) #1648

oliviertassinari opened this issue Apr 12, 2020 · 5 comments

Comments

@oliviertassinari
Copy link
Member

Environment

Tech Version
@material-ui/pickers v4.0.0-alpha.5

Steps to reproduce

https://next.material-ui-pickers.dev/demo/datepicker#basic-usage

Actual behavior

Capture d’écran 2020-04-13 à 00 11 25

Expected behavior

Capture d’écran 2020-04-13 à 00 07 00

  1. The component should have a higher density on desktop. It should be 258x281 instead of 320x358.
  2. The popper should be positioned to start on the right side, not at the center.
  3. The day chip should be 28x28 (maybe less, not sure) instead of 36x36 pixels.
  4. The year chip should be 54x28 instead of 72x36 pixels.
  5. The colors of the range don't match. Try to set the same primary color as in material.io #7001f3 to get an idea of the issue. I propose this diff:
diff --git a/lib/src/DateRangePicker/DateRangePickerDay.tsx b/lib/src/DateRangePicker/DateRangePickerDay.tsx
index 9f54cb31..bff27496 100644
--- a/lib/src/DateRangePicker/DateRangePickerDay.tsx
+++ b/lib/src/DateRangePicker/DateRangePickerDay.tsx
@@ -2,7 +2,7 @@ import * as React from 'react';
 import clsx from 'clsx';
 import { DAY_MARGIN } from '../constants/dimensions';
 import { useUtils } from '../_shared/hooks/useUtils';
-import { makeStyles, fade } from '@material-ui/core/styles';
+import { makeStyles, fade, lighten } from '@material-ui/core/styles';
 import { Day, DayProps, areDayPropsEqual } from '../views/Calendar/Day';

 interface DateRangeDayProps extends DayProps {
@@ -38,8 +38,8 @@ const useStyles = makeStyles(
     },
     rangeIntervalDayHighlight: {
       borderRadius: 0,
-      color: theme.palette.primary.contrastText,
-      backgroundColor: fade(theme.palette.primary.light, 0.6),
+      color: theme.palette.getContrastText(lighten(theme.palette.primary.main, 0.8)),
+      backgroundColor: lighten(theme.palette.primary.main, 0.8),
       '&:first-child': startBorderStyle,
       '&:last-child': endBorderStyle,
     },
@@ -66,7 +66,7 @@ const useStyles = makeStyles(
       },
     },
     dayInsideRangeInterval: {
-      color: theme.palette.getContrastText(fade(theme.palette.primary.light, 0.6)),
+      color: theme.palette.getContrastText(lighten(theme.palette.primary.main, 0.8)),
     },
     notSelectedDate: {
       backgroundColor: 'transparent',

before

Capture d’écran 2020-04-13 à 00 34 11

after

Capture d’écran 2020-04-13 à 00 33 18

baseline

Capture d’écran 2020-04-13 à 00 34 33

Looking again, 0.8 isn't enough. Maybe 0.85 or 0.9.

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Apr 13, 2020

Related to sizes. I am not sure – do core components precisely following sizes from guidelines? Because IMHO 32x32 is too small for mobile, isn't it?

image

@dmtrKovalenko
Copy link
Member

oh goodness, really components have different sizes for desktop. Looks like all of our sizes are for mobile. Thanks for catching it!

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 13, 2020

Sorry, I should have mentioned that mobile and desktop should have different dimensions. Thanks for inlining the screeshot from the specification with the actual dimensions.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 13, 2020

Regarding the media query logic. I would be eager to try a logic based on touch vs click support as the primary pointer device (instead of the screen width). It's definitely related to #1653.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 25, 2020

Related to sizes. I am not sure – do core components precisely following sizes from guidelines? Because IMHO 32x32 is too small for mobile, isn't it?

@dmtrKovalenko Regarding your concern about the size.

The core components try to follow the size of the specification, as much as possible, but we have divergences, especially when the specification isn't consistent or dropping 1px simplifies the CSS.

I think that we can benchmark with a couple of frequently used date pickers, to get a sense of where we are on the spectrum (sorted from too small to too large):

  • Google Calendar: visual: 24x24, bounding box: 36x28
  • ant.design: visual: 24x24, bounding box: 36x30
  • jQuery UI: visual: 34x24, bounding box: 36x26
  • daterangepicker.com visual/bounding box: 32x27
  • 💅 Material Design / Gmail: visual: 28x28, bounding box: 32x32
  • blueprintjs.com: visual/bounding box: 32x30
  • elastic.github.io/eui: visual/bounding box: 32x32
  • booking.com: visual/bounding box: 35x36
  • ⚾️ next.material-ui-pickers.dev: 36x36, bounding box: 40x40
  • Google flights: visual/bounding box: 44x44
  • airbnb.com: visual/bounding box: 48x47

Site note, we add vertical spacing on the rows and horizontal spacing on the columns. I think that we should aim for a single place (single element/selector) to handle CSS spacing, to make overrides simpler.

Capture d’écran 2020-04-25 à 13 12 24

Capture d’écran 2020-04-25 à 13 12 31

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

No branches or pull requests

2 participants