-
Notifications
You must be signed in to change notification settings - Fork 832
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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' }; | ||||||
|
||||||
|
@@ -58,6 +60,7 @@ export const DateTimePickerToolbar: React.FC<ToolbarComponentProps> = withDefaul | |||||
toolbarFormat, | ||||||
toolbarPlaceholder = '––', | ||||||
toolbarTitle = 'SELECT DATE & TIME', | ||||||
views, | ||||||
...other | ||||||
}) => { | ||||||
const utils = useUtils(); | ||||||
|
@@ -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' && ( | ||||||
|
@@ -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 | ||||||
tabIndex={-1} | ||||||
variant={timeTypographyText} | ||||||
data-mui-test="seconds" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering we want to move to
Suggested change
cc @mui-org/core-team. |
||||||
onClick={() => setOpenView('seconds')} | ||||||
selected={openView === 'seconds'} | ||||||
value={date ? utils.format(date, 'seconds') : '--'} | ||||||
typographyClassName={classes.timeTypography} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||
)} | ||||||
|
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 }); | ||||||
|
@@ -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'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, It's probably because we miss There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh nice, I'm adding this case to #1845. |
||||||
); | ||||||
}); | ||||||
}); |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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?