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

[Timeline] Migrate Timeline to emotion #25838

Merged
merged 9 commits into from
Apr 22, 2021
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Apr 19, 2021

One chunk of #24405

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 19, 2021

Details of bundle changes

@material-ui/lab: parsed: +0.11% , gzip: +0.04%

Generated by 🚫 dangerJS against 17d7c54

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Initial review done

packages/material-ui-lab/src/Timeline/Timeline.test.tsx Outdated Show resolved Hide resolved
{
name: 'MuiTimeline',
slot: 'Root',
// @ts-ignore TODO should fix the type in experimentalStyled
Copy link
Member

Choose a reason for hiding this comment

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

It's great that we can spot issues like this by actually using it in TS. Let me know if you need some help with this. Probably these can be inferred from the props' props, but also allow generic, as I think sometimes we have additional or calculated props here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mnajdova I fix as workaround by changing from string | object to Record<string, any>

@oliviertassinari oliviertassinari added the component: timeline This is the name of the generic UI component, not the React module! label Apr 19, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Some final comments

packages/material-ui-lab/src/Timeline/Timeline.test.tsx Outdated Show resolved Hide resolved
packages/material-ui-lab/src/Timeline/Timeline.tsx Outdated Show resolved Hide resolved
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.

This looks like our first migrated TypeScript component to styled. Nice to see it work, it will help with the date pickers

Comment on lines +43 to +47

/**
* className applied to the root element.
*/
className?: string;
Copy link
Member

@oliviertassinari oliviertassinari Apr 21, 2021

Choose a reason for hiding this comment

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

This is weird, I was expecting it to already come for free with React.HTMLAttributes<HTMLUListElement

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

👍

@mnajdova mnajdova merged commit 15ecbf8 into mui:next Apr 22, 2021
@siriwatknp siriwatknp deleted the timeline-emotion branch April 22, 2021 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: timeline 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.

4 participants