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

feat(core): update @ant-design/charts package to v2 for finefoods-antd, blog-issue-tracker examples #5523

Closed
wants to merge 13 commits into from

Conversation

devhik0
Copy link

@devhik0 devhik0 commented Jan 16, 2024

Updated @ant-design/charts from v1 to v2 for mentioned examples above

Closing issues

Self Check before Merge

Please check all items below before review.

  • Corresponding issues are created/updated or not needed
  • Docs are updated/provided or not needed
  • Examples are updated/provided or not needed
  • TypeScript definitions are updated/provided or not needed
  • Tests are updated/provided or not needed
  • Changesets are provided or not needed

Copy link

changeset-bot bot commented Jan 16, 2024

⚠️ No Changeset found

Latest commit: ef863ac

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

nx-cloud bot commented Jan 17, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4cc4da8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@BatuhanW BatuhanW marked this pull request as draft January 17, 2024 14:23
Copy link

netlify bot commented Jan 19, 2024

Deploy Preview for app-crm-minimal ready!

Name Link
🔨 Latest commit b35b608
🔍 Latest deploy log https://app.netlify.com/sites/app-crm-minimal/deploys/65ae60bc30a73f00080a4eff
😎 Deploy Preview https://deploy-preview-5523--app-crm-minimal.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@devhik0
Copy link
Author

devhik0 commented Jan 19, 2024

I dont know what else needed to complete this but as i understand i needed to add tests but because of i made a lot changes at different filles and not made big changes on that files, there is no additional tests. But i am thinking v2 of that lib is not breaking anything currently, so i dont know if tests required or not. Please guide me about that because i am new to codebase and dont want to break anything important.

documentation/package.json Show resolved Hide resolved
examples/blog-issue-tracker/src/components/task/pie.tsx Outdated Show resolved Hide resolved
examples/blog-issue-tracker/tsconfig.json Outdated Show resolved Hide resolved
examples/finefoods-antd/tsconfig.json Outdated Show resolved Hide resolved
examples/finefoods-antd/src/pages/dashboard/index.tsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/devtools-server/package.json Show resolved Hide resolved
@BatuhanW BatuhanW linked an issue Jan 22, 2024 that may be closed by this pull request
@BatuhanW BatuhanW added this to the February Release milestone Jan 22, 2024
@devhik0
Copy link
Author

devhik0 commented Jan 22, 2024

All in all, thanks for your interest and helps. I really appreciate that. I fixed some of your comments but some of them needs your decision, so i am waiting for your review, thanks. As a side note i removed loading prop from graphs because it was giving errors and library itself doesnt have that prop. Also i am getting some deprecation warnings about antd itself so i dont know if it needs to be also updated or not.

package.json Outdated Show resolved Hide resolved
packages/antd/src/hooks/table/useTable/useTable.ts Outdated Show resolved Hide resolved
documentation/package.json Show resolved Hide resolved
packages/live-previews/package.json Outdated Show resolved Hide resolved
@devhik0
Copy link
Author

devhik0 commented Jan 23, 2024

So i ve applied that changes you both suggested but want to push one commit for all. I am waiting for @omeraplak 's decisions for his review because they could require additional changes depends on current. At local everything looks okay for now.

@BatuhanW
Copy link
Member

So i ve applied that changes you both suggested but want to push one commit for all. I am waiting for @omeraplak 's decisions for his review because they could require additional changes depends on current. At local everything looks okay for now.

Hey @devhik0 don't worry about commits, we are squashing it before merge, as one commit already. You don't need to do anything special.

@BatuhanW
Copy link
Member

Hey @devhik0 I think this PR is too immature to be reviewed.

Disabling noImplicitAny or commenting out code blocks because they are throwing errors/warnings isn't way to go. PR should address exactly these issues.

I'm closing this PR for now, feel free to re-open it once you address the issues!

Thanks for the efforts, looking forward to your updates 🙌🏼

@BatuhanW BatuhanW closed this Jan 24, 2024
@devhik0
Copy link
Author

devhik0 commented Jan 24, 2024

So i fixed all issues and errors, tested locally both examples and docs, everything looks okay. I pushed them but i cant reopen this PR, should i add them in another PR or can you reopen this PR, thanks. @BatuhanW

@BatuhanW BatuhanW reopened this Jan 26, 2024
@devhik0
Copy link
Author

devhik0 commented Jan 26, 2024

Its ready for review i think

@devhik0 devhik0 marked this pull request as ready for review January 26, 2024 14:09
@devhik0 devhik0 requested review from a team as code owners January 26, 2024 14:09
@devhik0 devhik0 requested a review from BatuhanW January 26, 2024 14:18
@alicanerdurmaz
Copy link
Member

This PR contains changes in too many different packages and is very difficult to review. The scope of changes is too broad and lacks explanation. So I am closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants