Add seconds to DateTimePicker view #1961

JamesMHenderson · 2020-07-06T09:59:38Z

Add seconds as an available view in DateTimePickerView
Use DateTimePickerView type as the type for views and openTo
Change typography variant depending on whether seconds are being displayed or not
set number of grid elements depending on seconds view

[ x ] I have followed (at least) the PR section of the contributing guide.

cypress · 2020-07-06T10:04:08Z

Test summary

77 • 0 • 3 • 0

Run details


Project	material-ui-pickers
Status	Passed
Commit	`fcde662`
Started	Jul 8, 2020 9:37 AM
Ended	Jul 8, 2020 9:39 AM
Duration	02:17 💡
OS	Linux Debian - 10.0
Browser	Chrome 80

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/DateTimePicker/DateTimePicker.tsx

dmtrKovalenko

This PR missing a test case.

lib/src/DateTimePicker/DateTimePickerToolbar.tsx

dmtrKovalenko

Please fix all CI errors

JamesMHenderson · 2020-07-06T13:18:20Z

@dmtrKovalenko the CI was failing from the branch, I branched off 32a3a18.

How would I go about fixing the CI errors as they were not introduced by these changes?

oliviertassinari

Please rebase on the next branch

oliviertassinari · 2020-07-06T14:31:30Z

lib/src/DateTimePicker/DateTimePickerToolbar.tsx

            justify="center"
            alignItems="flex-end"
            direction={rtl ? 'row-reverse' : 'row'}
          >
            <ToolbarButton
-              variant="h3"
+              variant={timeTypographyText}


This could be an opportunity to fix the <hx> DOM output

What do you mean by this?

lib/src/__tests__/e2e/DateTimePickerRoot.test.tsx

oliviertassinari · 2020-07-06T14:32:24Z

lib/src/__tests__/e2e/DateTimePickerRoot.test.tsx

+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    component = mount(


Please prefer testing-library for this

I've based my test on existing tests which are using enzyme, is it prefered that the tests use testing-library over being consistent with the existing tests?

@JamesMHenderson the objective is to, step by step, be all in on testing-library. It requires all new tests to use it. This holds for all our codebase (core, pickers, etc). For instance, on the pickers side #1933.

oliviertassinari · 2020-07-06T14:33:16Z

lib/src/__tests__/e2e/DateTimePickerRoot.test.tsx

+        leftArrowIcon="keyboard_arrow_left"
+        rightArrowIcon="keyboard_arrow_right"
+        dateRangeIcon="date_range"
+        timeIcon="access_time"


This is not required and shouldn't work in the first place

Suggested change

leftArrowIcon="keyboard_arrow_left"

rightArrowIcon="keyboard_arrow_right"

dateRangeIcon="date_range"

timeIcon="access_time"

vercel · 2020-07-07T10:38:27Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/hgkprmjpe
✅ Preview: https://material-ui-pi-git-fork-jamesmhenderson-feature-date-tim-a37705.mui-org.vercel.app

- Add seconds as an available view in DateTimePickerView - Use DateTimePickerView type as the type for views and openTo - Change typography variant depending on whether seconds are being displayed or not - set number of grid elements depending on seconds view

oliviertassinari · 2020-07-08T10:09:37Z

lib/src/__tests__/DateTimePickerTestingLib.test.tsx

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


Suggested change

expect(getByMuiTest('seconds').firstChild?.textContent).toBe('05');

expect(getByMuiTest('seconds')).to.have.text('05');

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>>'.

Oh right, It's probably because we miss

https://github.com/mui-org/material-ui/blob/d1287f1874dffeade600364b36727fa7878e9a75/test/utils/initMatchers.js#L16

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

oliviertassinari · 2020-07-08T10:11:47Z

lib/src/__tests__/DateTimePickerTestingLib.test.tsx

+    fireEvent.click(getByMuiTest('seconds'));
+
+    expect(getByText(getByMuiTest('seconds'), '05')).toHaveClass(
+      'MuiPickersToolbarText-toolbarBtnSelected'


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

oliviertassinari · 2020-07-08T10:21:40Z

lib/src/DateTimePicker/DateTimePickerToolbar.tsx

+                    className={classes.separator}
+                  />
+
+                  <ToolbarButton


The HTLM output is wrong:

We shouldn't have any headers in here, as far as I know. If we want to, the h1 > h2 > h3… order is broken

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).

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?

oliviertassinari · 2020-07-08T10:22:14Z

lib/src/DateTimePicker/DateTimePickerToolbar.tsx

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


Suggested change

typographyClassName={classes.timeTypography}

className={classes.timeTypography}

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.

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

oliviertassinari · 2020-07-08T10:23:28Z

lib/src/DateTimePicker/DateTimePickerToolbar.tsx

+                  <ToolbarButton
+                    tabIndex={-1}
+                    variant={timeTypographyText}
+                    data-mui-test="seconds"


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.

eglavin · 2020-09-16T23:16:15Z

Hi fella any progress on this pull request? Anything i might be able to help with? Id love to implement this change into the app im working on, so if i can be of any help id be able to give some time!

oliviertassinari · 2020-09-18T10:06:55Z

@eglavin We are moving the date picker components into the lab. We will archive this repository once done. Any effort will need to be rebased from the main repository.

oliviertassinari · 2020-11-22T10:43:25Z

@JamesMHenderson Thanks for working on this problem. I'm closing as we have moved the component to https://github.com/mui-org/material-ui.

JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from bc37e4c to 17a4e46 Compare July 6, 2020 10:22

JamesMHenderson marked this pull request as draft July 6, 2020 10:24

JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from 17a4e46 to a53a800 Compare July 6, 2020 10:37

dmtrKovalenko reviewed Jul 6, 2020

View reviewed changes

lib/src/DateTimePicker/DateTimePicker.tsx Outdated Show resolved Hide resolved

dmtrKovalenko requested changes Jul 6, 2020

View reviewed changes

lib/src/DateTimePicker/DateTimePickerToolbar.tsx Outdated Show resolved Hide resolved

JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from a53a800 to 8bd6e61 Compare July 6, 2020 11:43

JamesMHenderson requested a review from dmtrKovalenko July 6, 2020 11:50

dmtrKovalenko requested changes Jul 6, 2020

View reviewed changes

oliviertassinari requested changes Jul 6, 2020

View reviewed changes

JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from 8bd6e61 to ccbd2fb Compare July 7, 2020 10:38

vercel bot deployed to Preview July 7, 2020 10:38 View deployment

JamesMHenderson changed the base branch from v3-x to next July 7, 2020 10:39

JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from ccbd2fb to 68d0c54 Compare July 7, 2020 11:34

vercel bot deployed to Preview July 7, 2020 11:34 View deployment

JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from 68d0c54 to fcde662 Compare July 8, 2020 09:33

vercel bot deployed to Preview July 8, 2020 09:33 View deployment

JamesMHenderson marked this pull request as ready for review July 8, 2020 09:35

JamesMHenderson requested a review from oliviertassinari July 8, 2020 09:36

oliviertassinari reviewed Jul 8, 2020

View reviewed changes

eglavin mentioned this pull request Sep 17, 2020

add seconds for DateTimerPicker #2097

Closed

1 task

oliviertassinari closed this Nov 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add seconds to DateTimePicker view #1961

Add seconds to DateTimePicker view #1961

JamesMHenderson commented Jul 6, 2020

cypress bot commented Jul 6, 2020 •

edited

Loading

dmtrKovalenko left a comment

dmtrKovalenko left a comment

JamesMHenderson commented Jul 6, 2020

oliviertassinari left a comment

oliviertassinari Jul 6, 2020

JamesMHenderson Jul 7, 2020

oliviertassinari Jul 6, 2020

JamesMHenderson Jul 7, 2020

oliviertassinari Jul 7, 2020

oliviertassinari Jul 6, 2020

vercel bot commented Jul 7, 2020 •

edited

Loading

oliviertassinari Jul 8, 2020

JamesMHenderson Jul 8, 2020

oliviertassinari Jul 8, 2020

oliviertassinari Jul 8, 2020

oliviertassinari Jul 8, 2020

oliviertassinari Jul 8, 2020

JamesMHenderson Jul 10, 2020

oliviertassinari Jul 8, 2020

JamesMHenderson Jul 10, 2020

oliviertassinari Jul 10, 2020

oliviertassinari Jul 8, 2020

eglavin commented Sep 16, 2020

oliviertassinari commented Sep 18, 2020

oliviertassinari commented Nov 22, 2020

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

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

Add seconds to DateTimePicker view #1961

Add seconds to DateTimePicker view #1961

Conversation

JamesMHenderson commented Jul 6, 2020

cypress bot commented Jul 6, 2020 • edited Loading

Test summary

Run details

dmtrKovalenko left a comment

Choose a reason for hiding this comment

dmtrKovalenko left a comment

Choose a reason for hiding this comment

JamesMHenderson commented Jul 6, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eglavin commented Sep 16, 2020

oliviertassinari commented Sep 18, 2020

oliviertassinari commented Nov 22, 2020

cypress bot commented Jul 6, 2020 •

edited

Loading

vercel bot commented Jul 7, 2020 •

edited

Loading