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][lab] Fix position alternate-reverse generated classname #37678

Conversation

ZeeshanTamboli
Copy link
Member

The classname generated for alternate-reverse class after adding it in #37311 should be .MuiTimeline-positionAlternateReverse, not .MuiTimeline-positionAlternate-reverse. The capitalize function only capitalizes the first letter of the first word and does not convert kebab-case to PascalCase.

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work package: lab Specific to @mui/lab component: timeline This is the name of the generic UI component, not the React module! labels Jun 22, 2023
@mui-bot
Copy link

mui-bot commented Jun 22, 2023

Netlify deploy preview

https://deploy-preview-37678--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 2d3a4d4

@ZeeshanTamboli ZeeshanTamboli added the regression A bug, but worse label Jun 22, 2023
Comment on lines +3 to +6
export default function convertTimelinePositionToClass(position: string): string {
return position === 'alternate-reverse'
? 'positionAlternateReverse'
: `position${capitalize(position)}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unsure whether I should create a pascalize function specifically to convert kebab-case to PascalCase for this situation or simply keep the string positionAlternateReverse hardcoded here.

Copy link
Member

Choose a reason for hiding this comment

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

We can add a function to convert kebab case to pascal case in the utils package and use it in all occurrences where needed. Nice catch, btw 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Since there is only one occurrence in the whole code-base, would it be worth it to create a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked and found that it is only needed here. I think it is not worth it to create a util function just for this and so I am keeping the string hard-coded.

@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review June 22, 2023 15:53
@ZeeshanTamboli ZeeshanTamboli merged commit f864cb4 into mui:master Jun 27, 2023
18 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the fix-timeline-position-alternate-reverse-classname branch June 27, 2023 06:37
@Sutar210599
Copy link

Hi @ZeeshanTamboli , is this the cause of Type '"alternate-reverse"' is not assignable to type '"left" | "right" | "alternate" error? The error is reproducible in the demo sandbox .

@ZeeshanTamboli ZeeshanTamboli changed the title [Timeline] Fix position alternate-reverse generated classname [Timeline][lab] Fix position alternate-reverse generated classname Jun 27, 2023
@ZeeshanTamboli
Copy link
Member Author

@Sutar210599 It is not related to this fix. The issue is that we added this feature in #37311 in the last release but we forgot to bump the version of @mui/lab package in v5.13.6 latest release - #37686.

@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Jun 27, 2023
@Sutar210599
Copy link

Ok, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: timeline This is the name of the generic UI component, not the React module! package: lab Specific to @mui/lab regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants