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.

Sign up for GitHub

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

Better recognition of touch devices #1653

Merged
merged 6 commits into from
Apr 21, 2020
Merged

Better recognition of touch devices #1653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab5c99b
Change breakpoint when desktop mode will be enabled for ResponsiveWra…
dmtrKovalenko Apr 13, 2020
77ff855
Use media hover to better determine touch devices
dmtrKovalenko Apr 16, 2020
ee842a5
Fix incorrect type
dmtrKovalenko Apr 16, 2020
b2c78b6
Use real touch device for pickers visual regression
dmtrKovalenko Apr 16, 2020
b09ae11
Change touch device media to (hover: fine)
dmtrKovalenko Apr 21, 2020
7c61217
Remove unused import
dmtrKovalenko Apr 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions docs/prop-types.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions e2e/integration/VisualRegression.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('Visual Regression', () => {
{
url: '/demo/datepicker',
name: 'DatePicker demo',
hardResponsive: true,
withRealTouchDevice: true,
withDarkTheme: true,
scenarios: {
'Opened datepicker': () => {
Expand All @@ -26,7 +26,7 @@ describe('Visual Regression', () => {
{
url: '/demo/timepicker',
name: 'TimePicker demo',
responsive: true,
withRealTouchDevice: true,
withDarkTheme: true,
scenarios: {
'Opened timepicker': () => {
Expand All @@ -40,7 +40,7 @@ describe('Visual Regression', () => {
{
url: '/demo/datetime-picker',
name: 'DateTimePicker demo',
responsive: true,
withRealTouchDevice: true,
withDarkTheme: true,
scenarios: {
'Opened datetimepicker': () => {
Expand Down Expand Up @@ -88,7 +88,7 @@ describe('Visual Regression', () => {
}

if (page.scenarios) {
const defaultWidthForScenarios = page.hardResponsive ? [1280] : [1280, 375];
const defaultWidthForScenarios = page.withRealTouchDevice ? [1280] : [1280, 375];

Object.entries(page.scenarios).forEach(([name, execute]) => {
if (!execute || typeof execute !== 'function') {
Expand All @@ -101,7 +101,7 @@ describe('Visual Regression', () => {
cy.percySnapshot(`${page.name}: ${name}`, { widths: defaultWidthForScenarios });
});

if (page.hardResponsive) {
if (page.withRealTouchDevice) {
it(`${page.name} scenario: ${name} on mobile`, () => {
cy.viewport('iphone-x');

Expand All @@ -119,7 +119,7 @@ describe('Visual Regression', () => {
});
}

if (page.withDarkTheme && page.hardResponsive) {
if (page.withDarkTheme && page.withRealTouchDevice) {
it(`${page.name} scenario: ${name} on mobile in dark theme`, () => {
cy.viewport('iphone-x');
cy.toggleTheme();
Expand Down
2 changes: 1 addition & 1 deletion lib/src/DateRangePicker/DateRangePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import { parsePickerInputValue } from '../_helpers/date-utils';
import { usePickerState } from '../_shared/hooks/usePickerState';
import { AllSharedPickerProps } from '../Picker/SharedPickerProps';
import { DateRange as DateRangeType, RangeInput } from './RangeTypes';
import { ResponsivePopperWrapper } from '../wrappers/ResponsiveWrapper';
import { DesktopPopperWrapper } from '../wrappers/DesktopPopperWrapper';
import { MuiPickersAdapter, useUtils } from '../_shared/hooks/useUtils';
import { makeWrapperComponent } from '../wrappers/makeWrapperComponent';
import { ResponsivePopperWrapper } from '../wrappers/ResponsiveWrapper';
import { SomeWrapper, ExtendWrapper, StaticWrapper } from '../wrappers/Wrapper';
import { DateRangePickerView, ExportedDateRangePickerViewProps } from './DateRangePickerView';
import { DateRangePickerInput, ExportedDateRangePickerInputProps } from './DateRangePickerInput';
Expand Down
2 changes: 1 addition & 1 deletion lib/src/__tests__/DatePicker.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('e2e - DatePicker default year format', () => {
<DatePicker
DialogProps={{}}
PopoverProps={{}}
desktopModeBreakpoint="xs"
desktopModeMediaQuery="(min-width:720px)"
value={utilsToUse.date('2018-01-01T00:00:00.000')}
onChange={onChangeMock}
views={['year']}
Expand Down
2 changes: 2 additions & 0 deletions lib/src/constants/dimensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ export const VIEW_HEIGHT = 358;
export const DAY_SIZE = 36;

export const DAY_MARGIN = 2;

export const IS_TOUCH_DEVICE_MEDIA = '@media (pointer: fine)';
3 changes: 2 additions & 1 deletion lib/src/wrappers/DesktopPopperWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { makeStyles } from '@material-ui/core/styles';
import { InnerMobileWrapperProps } from './MobileWrapper';
import { InnerDesktopWrapperProps } from './DesktopWrapper';
import { WrapperVariantContext } from './WrapperVariantContext';
import { IS_TOUCH_DEVICE_MEDIA } from '../constants/dimensions';
import { KeyboardDateInput } from '../_shared/KeyboardDateInput';
import { useGlobalKeyDown, keycode } from '../_shared/hooks/useKeyDown';
import { TransitionProps } from '@material-ui/core/transitions/transition';
Expand All @@ -36,7 +37,7 @@ const useStyles = makeStyles(theme => ({
transformOrigin: 'top center',
'&:focus': {
outline: 'auto',
'@media (pointer:coarse)': {
[IS_TOUCH_DEVICE_MEDIA]: {
outline: 0,
},
},
Expand Down
3 changes: 2 additions & 1 deletion lib/src/wrappers/DesktopWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { StaticWrapperProps } from './StaticWrapper';
import { makeStyles } from '@material-ui/core/styles';
import { InnerMobileWrapperProps } from './MobileWrapper';
import { WrapperVariantContext } from './WrapperVariantContext';
import { IS_TOUCH_DEVICE_MEDIA } from '../constants/dimensions';
import { InnerDesktopPopperWrapperProps } from './DesktopPopperWrapper';

export interface InnerDesktopWrapperProps {
Expand All @@ -23,7 +24,7 @@ const useStyles = makeStyles({
popover: {
'&:focus': {
outline: 'auto',
'@media (pointer:coarse)': {
[IS_TOUCH_DEVICE_MEDIA]: {
outline: 0,
},
},
Expand Down
15 changes: 7 additions & 8 deletions lib/src/wrappers/ResponsiveWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,27 @@
import * as React from 'react';
import useMediaQuery from '@material-ui/core/useMediaQuery';
import { useTheme } from '@material-ui/core/styles';
import { IS_TOUCH_DEVICE_MEDIA } from '../constants/dimensions';
import { MobileWrapperProps, MobileWrapper } from './MobileWrapper';
import { DesktopWrapperProps, DesktopWrapper } from './DesktopWrapper';
import { Breakpoint } from '@material-ui/core/styles/createBreakpoints';
import { DesktopPopperWrapperProps, DesktopPopperWrapper } from './DesktopPopperWrapper';

export interface ResponsiveWrapperProps
extends DesktopWrapperProps,
DesktopPopperWrapperProps,
MobileWrapperProps {
/** Breakpoint when `Desktop` mode will be changed to `Mobile`
* @default 'md'
/** Css media query when `Mobile` mode will be changed to `Desktop`
* @default "@media (pointer: fine)"
* @example "@media (min-width: 720px)" or theme.breakpoints.up("sm")
Comment on lines +12 to +14
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of this variation to better match the style of the mono-repository for the description of the props?

Suggested change
/** Css media query when `Mobile` mode will be changed to `Desktop`
* @default "@media (pointer: fine)"
* @example "@media (min-width: 720px)" or theme.breakpoints.up("sm")
/**
* CSS media query when "mobile" mode will be changed to "desktop".
*
* @default "@media (pointer: fine)"
* @example "@media (min-width: 720px)" or theme.breakpoints.up("sm")

Copy link
Member Author

Choose a reason for hiding this comment

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

Mobile and Desktop are used as prefixes for specific components (e.g. MobileDatePicker) so it should be more clear for users. Isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mobile and Desktop are used as prefixes for specific components (e.g. MobileDatePicker) so it should be more clear for users. Isn't it?

Copy link
Member

@oliviertassinari oliviertassinari Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Using Mobile feels like it's a piece of code that can be used on its own. What about 1. MobileDatePicker or 2. "mobile", I think that we would lose value if we try to compromise between these two options.

*/
desktopModeBreakpoint?: Breakpoint;
desktopModeMediaQuery?: string;
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 a shorter name for the prop?

Suggested change
desktopModeMediaQuery?: string;
desktopMediaQuery?: string;

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the shorter variant is less expressive. What "desktop" means?

Copy link
Member

Choose a reason for hiding this comment

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

It could be interpreted as "a media query for desktop", definitely not as good as the information we can provide in a description. Adding mode definitely provide extra context. I don't know. I was wondering about this tradeoff of information vs length for the props, e.g. openPickerIcon, vs pickerIcon.

}

export const makeResponsiveWrapper = (
DesktopWrapperComponent: React.FC<DesktopWrapperProps | DesktopPopperWrapperProps>,
MobileWrapperComponent: React.FC<MobileWrapperProps>
) => {
const ResponsiveWrapper: React.FC<ResponsiveWrapperProps> = ({
desktopModeBreakpoint = 'md',
desktopModeMediaQuery = IS_TOUCH_DEVICE_MEDIA,
okLabel,
cancelLabel,
clearLabel,
Expand All @@ -35,8 +35,7 @@ export const makeResponsiveWrapper = (
displayStaticWrapperAs,
...other
}) => {
const theme = useTheme();
const isDesktop = useMediaQuery(theme.breakpoints.up(desktopModeBreakpoint));
const isDesktop = useMediaQuery(desktopModeMediaQuery);

return isDesktop ? (
<DesktopWrapperComponent
Expand Down