Skip to content

[Timeline] Migrate Timeline to emotion#25838

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

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

Conversation

@siriwatknp

@siriwatknp siriwatknp commented Apr 19, 2021

Copy link
Copy Markdown
Member

One chunk of #24405

@mui-pr-bot

mui-pr-bot commented Apr 19, 2021

Copy link
Copy Markdown

Details of bundle changes

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

Generated by 🚫 dangerJS against 17d7c54

@mnajdova mnajdova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Initial review done

Comment thread packages/material-ui-lab/src/Timeline/Timeline.test.tsx Outdated
{
name: 'MuiTimeline',
slot: 'Root',
// @ts-ignore TODO should fix the type in experimentalStyled

Copy link
Copy Markdown
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
Copy Markdown
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>

Comment thread packages/material-ui-unstyled/src/composeClasses/composeClasses.ts
@oliviertassinari oliviertassinari added the scope: timeline Changes related to the timeline. label Apr 19, 2021
Comment thread packages/material-ui/src/styles/experimentalStyled.d.ts

@mnajdova mnajdova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some final comments

Comment thread packages/material-ui-lab/src/Timeline/Timeline.test.tsx Outdated
Comment thread packages/material-ui-lab/src/Timeline/Timeline.tsx Outdated

@oliviertassinari oliviertassinari left a comment

Copy link
Copy Markdown
Member

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;

@oliviertassinari oliviertassinari Apr 21, 2021

Copy link
Copy Markdown
Member

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

Comment thread packages/material-ui-lab/src/Timeline/Timeline.tsx
Comment thread packages/material-ui-lab/src/Timeline/Timeline.tsx Outdated

@mnajdova mnajdova left a comment

Copy link
Copy Markdown
Member

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