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

Conversation

JamesMHenderson
Copy link

  • 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

@cypress
Copy link

cypress bot commented Jul 6, 2020



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

@JamesMHenderson JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from bc37e4c to 17a4e46 Compare July 6, 2020 10:22
@JamesMHenderson JamesMHenderson marked this pull request as draft July 6, 2020 10:24
@JamesMHenderson JamesMHenderson force-pushed the feature/date-time-picker-seconds branch from 17a4e46 to a53a800 Compare July 6, 2020 10:37
Copy link
Member

@dmtrKovalenko dmtrKovalenko left a comment

Choose a reason for hiding this comment

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

This PR missing a test case.

lib/src/DateTimePicker/DateTimePickerToolbar.tsx Outdated Show resolved Hide resolved
Copy link
Member

@dmtrKovalenko dmtrKovalenko left a comment

Choose a reason for hiding this comment

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

Please fix all CI errors

@JamesMHenderson
Copy link
Author

@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?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Please rebase on the next branch

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by this?

lib/src/__tests__/e2e/DateTimePickerRoot.test.tsx Outdated Show resolved Hide resolved

beforeEach(() => {
jest.clearAllMocks();
component = mount(
Copy link
Member

Choose a reason for hiding this comment

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

Please prefer testing-library for this

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 86 to 89
leftArrowIcon="keyboard_arrow_left"
rightArrowIcon="keyboard_arrow_right"
dateRangeIcon="date_range"
timeIcon="access_time"
Copy link
Member

Choose a reason for hiding this comment

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

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"

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

vercel bot commented Jul 7, 2020

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

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.

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.

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?

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 '

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

@eglavin
Copy link

eglavin commented Sep 16, 2020

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!

@eglavin eglavin mentioned this pull request Sep 17, 2020
1 task
@oliviertassinari
Copy link
Member

@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
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

None yet

4 participants