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

[DateRangePicker] Migrate internal components to emotion #26326

Merged
merged 8 commits into from
May 19, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented May 16, 2021

These internal components follow the same approach for migration so I think it is easier to bundle them together to review.

@siriwatknp siriwatknp added the component: date range picker This is the name of the generic UI component, not the React module! label May 16, 2021
@siriwatknp siriwatknp requested a review from mnajdova May 16, 2021 16:29
@mui-pr-bot
Copy link

mui-pr-bot commented May 16, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against b25f23b

@siriwatknp
Copy link
Member Author

siriwatknp commented May 16, 2021

@mnajdova One thing that I wonder after did some of the migration for internal components. It seems like the second one is simpler and gives the same result as the first one?

const Root = styled('div', {}, { skipSx: true })

const Root = styled('div')

@siriwatknp siriwatknp force-pushed the date-range-picker-emotion branch from 52e18b8 to 113ea0f Compare May 16, 2021 16:46
@siriwatknp
Copy link
Member Author

Argos fail due to PickersCalendar is not migrate to emotion and css from withStyles has higher priority. will open another PR to migrate PickersCalendar first.

@@ -1,7 +1,6 @@
import * as React from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

Include PickersCalendar here because of style override (min-height) in DateRangePickerViewDesktop

Comment on lines +60 to +62
const weeksContainerHeight = (DAY_SIZE + DAY_MARGIN * 4) * 6;

// TODO remove PickersCalendarClassKey in CalendarPickerSkeleton migration
Copy link
Member Author

Choose a reason for hiding this comment

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

Cannot remove PickersCalendarClassKey and styles in this PR due to dependency to CalendarPickerSkeleton. will removed them in CalendarPickerSkeleton migration PR

@mnajdova
Copy link
Member

@mnajdova One thing that I wonder after did some of the migration for internal components. It seems like the second one is simpler and gives the same result as the first one?

const Root = styled('div', {}, { skipSx: true })

const Root = styled('div')

What do you mean result as the first one? The default value for skipSx is false: https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/styles/experimentalStyled.js#L81 Are you suggesting we change the default or?

@siriwatknp
Copy link
Member Author

@mnajdova One thing that I wonder after did some of the migration for internal components. It seems like the second one is simpler and gives the same result as the first one?

const Root = styled('div', {}, { skipSx: true })

const Root = styled('div')

What do you mean result as the first one? The default value for skipSx is false: https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/styles/experimentalStyled.js#L81 Are you suggesting we change the default or?

My bad, didn't explain it clearly. I mean why do we need to have skipSx option here, is it help on the performance if skipSx is provided?.

@siriwatknp siriwatknp requested a review from mnajdova May 18, 2021 03:27
@mnajdova
Copy link
Member

My bad, didn't explain it clearly. I mean why do we need to have skipSx option here, is it help on the performance if skipSx is provided?

We discussed it in one of the first pickers PR #25989 In my opinion we don't need to do it, but let's follow the convention it is at this moment, we can update before the beta version if we decide it's ok to have it on the internal components as well.

@siriwatknp siriwatknp merged commit 9bf937a into mui:next May 19, 2021
@siriwatknp siriwatknp deleted the date-range-picker-emotion branch May 19, 2021 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: date range picker This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants