Skip to content

[Timeline] Migrate Timeline to emotion#25838

Merged
mnajdova merged 9 commits intomui:nextfrom
siriwatknp:timeline-emotion
Apr 22, 2021
Merged

[Timeline] Migrate Timeline to emotion#25838
mnajdova merged 9 commits intomui:nextfrom
siriwatknp:timeline-emotion

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

{
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 scope: timeline Changes related to the timeline. 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

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

scope: timeline Changes related to the timeline.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants