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

[material-ui][Badge] Deprecated components and componentsProps props for v6 #41399

Closed
wants to merge 34 commits into from
Closed

[material-ui][Badge] Deprecated components and componentsProps props for v6 #41399

wants to merge 34 commits into from

Conversation

skmanoj322
Copy link
Contributor

@skmanoj322 skmanoj322 commented Mar 7, 2024

@DiegoAndai part of #41279

@mui-bot
Copy link

mui-bot commented Mar 7, 2024

Netlify deploy preview

Bundle size report

Bundle size will be reported once CircleCI build #674699 finishes.

Generated by 🚫 dangerJS against 0583e74

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in the review.

Let me know if you need help with any of these 😊

@@ -283,31 +283,8 @@ describe('<Badge />', () => {
});
});

describe('prop: components / slots', () => {
Copy link
Member

Choose a reason for hiding this comment

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

No need to delete this, we will delete it when the prop is removed.

@@ -331,23 +308,7 @@ describe('<Badge />', () => {
});
});

describe('prop: componentsProps / slotProps', () => {
Copy link
Member

Choose a reason for hiding this comment

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

No need to delete this, we will delete it when the prop is removed.

Copy link
Member

Choose a reason for hiding this comment

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


slotProps: {
root: slotsRootProps,
badge: slotsbadgeProps,
Copy link
Member

@DiegoAndai DiegoAndai Mar 7, 2024

Choose a reason for hiding this comment

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

We need to update this one as well. Same as my previous comment (#41399 (comment)).

@zannager zannager added the component: badge This is the name of the generic UI component, not the React module! label Mar 8, 2024
@skmanoj322
Copy link
Contributor Author

@DiegoAndai made changes as told by you. please review and give feedback

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Hey @skmanoj322! Thanks for working on this. Here are some review comments:

@skmanoj322
Copy link
Contributor Author

Hey @DiegoAndai Thank you for bearing with me . I have refered the slider and alert component and implemented the same here. just a short question is there any particular logic to write these test (actual.js)

and ,pnpm tc <codemod> i tried to implement the command from the root terminal it gave me error.
can you give example how to implement these command?

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Hey @skmanoj322!

To fix the failing test cases:

  • Running pnpm prettier should modify the files with errors, and then commit and push the changes
  • I left code suggestions to fix the codemod test failures

just a short question is there any particular logic to write these test (actual.js)

The idea is to cover all ways users might be using the deprecated props. Then you should be able to run

node packages/mui-codemod/codemod deprecations/badge-props packages/mui-codemod/src/deprecations/badge-props/test-cases/actual.js

And that would modify actual.js, then you copy the modified code to expected.js and undo the modification of actual.js

pnpm tc i tried to implement the command from the root terminal it gave me error

Did you run this?:

pnpm tc badge-props

What error did it throw?

it('should be idempotent', () => {
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {});

const excepted = read('./test-cases/excepted.js');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const excepted = read('./test-cases/excepted.js');
const excepted = read('./test-cases/expected.js');

it('transforms props as needed', () => {
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {});

const expected = read('./test-cases/excepted.js');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const expected = read('./test-cases/excepted.js');
const expected = read('./test-cases/expected.js');

describe('deprecations', () => {
describe('badge-props', () => {
it('transforms props as needed', () => {
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {});
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, { printOptions: { trailingComma: true } });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these is missing in slider any specific reason why?

@skmanoj322
Copy link
Contributor Author

skmanoj322 commented Mar 14, 2024

hey @DiegoAndai tried to run these test

What error did it throw?

pnpm tc badge-props

pnpm  tc badge-props

and for

pnpm eslint. i think these will clear my ci/circleci: test_static

eslintError

btw really appricate your mentoring thanks for the guidence 🎉 👍

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 20, 2024
@skmanoj322
Copy link
Contributor Author

skmanoj322 commented Mar 25, 2024

@DiegoAndai sorry i think there is a lot to merge conflit i have to resolve that . again sorry for trouble again 😞 .

i thing there is lot of code changes that have happened . shall i just create new PR?

Or
is it right way

We will have to update this after #41323 was merged. For this, we need to merge master into this branch:

  1. git checkout master
  2. git pull upstream master
  3. git checkout manoj/badge-v6-deprecations
  4. git merge master
  5. Run the codemod on the actual.js file and copy it here

Let me know if you need help 😊

@DiegoAndai
Copy link
Member

Closing in favor of #41655

@DiegoAndai DiegoAndai closed this Mar 26, 2024
@DiegoAndai
Copy link
Member

@skmanoj322 sorry I was late in replying to this PR. The issue that caused so many files to change is that we recently switched to the next branch, so in this instructions:

  1. git checkout master
  2. git pull upstream master
  3. git checkout manoj/badge-v6-deprecations
  4. git merge master
  5. Run the codemod on the actual.js file and copy it here

Instad of master, we should've merged next. But the other PR is approved now 👍🏼

@skmanoj322 skmanoj322 deleted the manoj/badge-v6-deprecations branch March 28, 2024 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: badge This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants