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

Add seconds to DateTimePicker view #1961

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions docs/pages/demo/datetime-picker/CustomDateTimePicker.example.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ function CustomDateTimePicker(props) {
onChange={handleClearedDateChange}
renderInput={props => <TextField {...props} helperText="Clear Initial State" />}
/>

<DateTimePicker
value={selectedDate}
onChange={handleDateChange}
renderInput={props => <TextField {...props} helperText="With Seconds" />}
views={['year', 'month', 'date', 'hours', 'minutes', 'seconds']}
inputFormat={props.__willBeReplacedGetFormatString({
moment: 'YYYY/MM/DD HH:mm:ss A',
dateFns: 'yyyy/MM/dd HH:mm:ss a',
})}
/>
</React.Fragment>
);
}
Expand Down
28 changes: 14 additions & 14 deletions docs/prop-types.json

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

2 changes: 1 addition & 1 deletion lib/src/DateTimePicker/DateTimePicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { ParsableDate, defaultMinDate, defaultMaxDate } from '../constants/prop-
export type DateTimePickerView = 'year' | 'date' | 'month' | 'hours' | 'minutes' | 'seconds';

export interface BaseDateTimePickerProps
extends WithViewsProps<'year' | 'date' | 'month' | 'hours' | 'minutes'>,
extends WithViewsProps<'year' | 'date' | 'month' | 'hours' | 'minutes' | 'seconds'>,
ValidationProps<DateAndTimeValidationError, ParsableDate>,
ExportedClockViewProps,
ExportedCalendarViewProps {
Expand Down
32 changes: 29 additions & 3 deletions lib/src/DateTimePicker/DateTimePickerToolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import { MaterialUiPickersDate } from '../typings/date';
import { withDefaultProps } from '../_shared/withDefaultProps';
import { ToolbarComponentProps } from '../Picker/SharedPickerProps';
import { WrapperVariantContext } from '../wrappers/WrapperVariantContext';
import { useMemo } from 'react';
import { arrayIncludes } from '../_helpers/utils';

const muiComponentConfig = { name: 'MuiPickersDateTimePickerToolbar' };

Expand Down Expand Up @@ -58,6 +60,7 @@ export const DateTimePickerToolbar: React.FC<ToolbarComponentProps> = withDefaul
toolbarFormat,
toolbarPlaceholder = '––',
toolbarTitle = 'SELECT DATE & TIME',
views,
...other
}) => {
const utils = useUtils();
Expand All @@ -83,6 +86,9 @@ export const DateTimePickerToolbar: React.FC<ToolbarComponentProps> = withDefaul
return utils.format(date, 'shortDate');
}, [date, toolbarFormat, toolbarPlaceholder, utils]);

const hasSeconds = useMemo(() => arrayIncludes(views, 'seconds'), [views]);
const timeTypographyText = hasSeconds ? 'h4' : 'h3';

return (
<React.Fragment>
{wrapperVariant !== 'desktop' && (
Expand Down Expand Up @@ -117,25 +123,45 @@ export const DateTimePickerToolbar: React.FC<ToolbarComponentProps> = withDefaul
<div className={classes.timeContainer}>
<ToolbarButton
tabIndex={-1}
variant="h3"
variant={timeTypographyText}
data-mui-test="hours"
onClick={() => setOpenView('hours')}
selected={openView === 'hours'}
value={date ? formatHours(date) : '--'}
typographyClassName={classes.timeTypography}
/>

<ToolbarText variant="h3" value=":" className={classes.separator} />
<ToolbarText variant={timeTypographyText} value=":" className={classes.separator} />

<ToolbarButton
tabIndex={-1}
variant="h3"
variant={timeTypographyText}
data-mui-test="minutes"
onClick={() => setOpenView('minutes')}
selected={openView === 'minutes'}
value={date ? utils.format(date, 'minutes') : '--'}
typographyClassName={classes.timeTypography}
/>

{hasSeconds && (
<>
<ToolbarText
variant={timeTypographyText}
value=":"
className={classes.separator}
/>

<ToolbarButton
Copy link
Member

Choose a reason for hiding this comment

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

The HTLM output is wrong:

Capture d’écran 2020-07-08 à 12 19 08

  1. We shouldn't have any headers in here, as far as I know. If we want to, the h1 > h2 > h3… order is broken
  2. button > span > h4 nesting structure is wrong. I propose we remove the h4 element (we can use the component prop to compose behavior (button) and style (text).

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you, but I'm not sure what the best approach would be for this. Would it be best to pick this sort of change up seperately from this pull request as it would be a significant refactor?

tabIndex={-1}
variant={timeTypographyText}
data-mui-test="seconds"
Copy link
Member

Choose a reason for hiding this comment

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

Considering we want to move to data-testid in the whole codebase, I suggest we start here

Suggested change
data-mui-test="seconds"
data-testid="seconds"

cc @mui-org/core-team.

onClick={() => setOpenView('seconds')}
selected={openView === 'seconds'}
value={date ? utils.format(date, 'seconds') : '--'}
typographyClassName={classes.timeTypography}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
typographyClassName={classes.timeTypography}
className={classes.timeTypography}

Copy link
Author

Choose a reason for hiding this comment

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

The Toolbar button has a prop typography class which is passed in to add the class to the typography component in the ToolbarButton, rather than adding the class to the Button directly.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there is an opportunity here to flatten the DOM structure '

/>
</>
)}
</div>
</PickerToolbar>
)}
Expand Down
43 changes: 40 additions & 3 deletions lib/src/__tests__/DateTimePickerTestingLib.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as React from 'react';
import { utilsToUse } from './test-utils';
import { getByMuiTest, utilsToUse } from './test-utils';
import TextField from '@material-ui/core/TextField';
import { DesktopDateTimePicker } from '../DateTimePicker';
import { DesktopDateTimePicker, MobileDateTimePicker } from '../DateTimePicker';
import { createClientRender } from './createClientRender';
import { fireEvent, screen, waitFor } from '@testing-library/react';
import { fireEvent, getByText, screen, waitFor } from '@testing-library/react';

describe('<DateTimePicker />', () => {
const render = createClientRender({ strict: false });
Expand Down Expand Up @@ -63,4 +63,41 @@ describe('<DateTimePicker />', () => {
await waitFor(() => screen.getByRole('dialog'));
expect(screen.getByLabelText('11 hours').getAttribute('aria-disabled')).toBe('true');
});

it('prop: views – seconds is visible', async () => {
render(
<MobileDateTimePicker
open
onChange={() => {}}
ampm={false}
renderInput={props => <TextField {...props} />}
value={utilsToUse.date('2018-01-02T03:04:05.000Z')}
views={['year', 'month', 'date', 'hours', 'minutes', 'seconds']}
/>
);

await waitFor(() => screen.getByRole('dialog'));
expect(getByMuiTest('seconds').firstChild?.textContent).toBe('05');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(getByMuiTest('seconds').firstChild?.textContent).toBe('05');
expect(getByMuiTest('seconds')).to.have.text('05');

Copy link
Author

Choose a reason for hiding this comment

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

I don't think this is valid

I get a type error with this change. Property 'to' does not exist on type 'JestMatchersShape , Matchers , string | null>>'.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, forget about it, we will have it resolved by merging the repository in the main one.

});

it('views: clicking on seconds opens seconds view', async () => {
render(
<MobileDateTimePicker
open
onChange={() => {}}
ampm={false}
renderInput={props => <TextField {...props} />}
value={utilsToUse.date('2018-01-02T03:04:05.000Z')}
views={['year', 'month', 'date', 'hours', 'minutes', 'seconds']}
/>
);

await waitFor(() => screen.getByRole('dialog'));

fireEvent.click(getByMuiTest('seconds'));

expect(getByText(getByMuiTest('seconds'), '05')).toHaveClass(
'MuiPickersToolbarText-toolbarBtnSelected'
Copy link
Member

Choose a reason for hiding this comment

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

Oh nice, I'm adding this case to #1845.

);
});
});
4 changes: 2 additions & 2 deletions lib/src/__tests__/test-utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function getAllByMuiTest(
id: Matcher,
container: HTMLElement = document.body,
options?: MatcherOptions
): Element[] {
): HTMLElement[] {
const els = queryAllByMuiTest(container, id, options);
if (!els.length) {
throw queryHelpers.getElementError(
Expand All @@ -31,7 +31,7 @@ export function getAllByMuiTest(
return els;
}

export function getByMuiTest(...args: Parameters<typeof getAllByMuiTest>): Element {
export function getByMuiTest(...args: Parameters<typeof getAllByMuiTest>): HTMLElement {
const result = getAllByMuiTest(...args);
if (result.length > 0) {
return result[0];
Expand Down