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
Added type definitions for @commerce7/admin-ui #69407
Added type definitions for @commerce7/admin-ui #69407
Conversation
@Weldawadyathink Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. 1 package in this PR
Code ReviewsThis PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 69407,
"author": "Weldawadyathink",
"headCommitOid": "5befebeaff14c54bc313c95457d6fa87fa0c6e39",
"mergeBaseOid": "37d70ca66abce5789684db705021e27aa00832f5",
"lastPushDate": "2024-04-21T04:41:51.000Z",
"lastActivityDate": "2024-05-17T20:03:11.000Z",
"mergeOfferDate": "2024-05-17T20:01:57.000Z",
"mergeRequestDate": "2024-05-17T20:03:11.000Z",
"mergeRequestUser": "Weldawadyathink",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "commerce7__admin-ui",
"kind": "add",
"files": [
{
"path": "types/commerce7__admin-ui/.npmignore",
"kind": "package-meta-ok"
},
{
"path": "types/commerce7__admin-ui/commerce7__admin-ui-tests.tsx",
"kind": "test"
},
{
"path": "types/commerce7__admin-ui/index.d.ts",
"kind": "definition"
},
{
"path": "types/commerce7__admin-ui/package.json",
"kind": "package-meta-ok"
},
{
"path": "types/commerce7__admin-ui/tsconfig.json",
"kind": "package-meta",
"suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-tsconfigjson) (check: `compilerOptions.lib.0`)"
}
],
"owners": [],
"addedOwners": [
"Weldawadyathink"
],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "sandersn",
"date": "2024-05-17T20:01:21.000Z",
"isMaintainer": true
},
{
"type": "stale",
"reviewer": "jakebailey",
"date": "2024-05-15T03:55:02.000Z",
"abbrOid": "8ca3d7f"
},
{
"type": "stale",
"reviewer": "rbuckton",
"date": "2024-05-03T14:56:30.000Z",
"abbrOid": "7f7777f"
}
],
"mainBotCommentID": 2067902261,
"ciResult": "pass"
} |
🔔 @Weldawadyathink — you're the only owner, but it would still be good if you find someone to review this PR in the next few days, otherwise a maintainer will look at it. (And if you do find someone, maybe even recruit them to be a second owner to make future changes easier...) |
you would have to answer typical questions:
|
Re-ping @«anyone?»: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
It isn't auto generated, I just didn't think about putting the eslint config inline. The only eslint I needed to override was only having one declare module statement. Let me know if there is anything else that needs a second pass, and I can update the PR. |
That's a fair point. The less you need to override in the eslint configuration, the better. |
@@ -0,0 +1,11 @@ | |||
{ | |||
"rules": { | |||
"@definitelytyped/no-any-union": "off", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per @peterblazejewicz, handling these inline or adjusting the types to not require disabling the lint rules would be preferred.
@Weldawadyathink One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
It has been more than two weeks and this PR still has no reviews. I'll bump it to the DT maintainer queue. Thank you for your patience, @Weldawadyathink. (Ping @«anyone?».) |
@Weldawadyathink The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
Updated to (hopefully) address all requests. The eslint configs are gone. After some discussion on the discord, I found out I don't need the |
@rbuckton Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
@@ -0,0 +1 @@ | |||
import { Commerce7AdminUI } from "@commerce7/admin-ui"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like not very many tests; should this be a .tsx
file and then actually use the components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will admit I didn't really know what tests to write. Do you know of a react component library that is similar that I could use for some guidance? I looked through the typings for material-ui, but that library seems so different than @commerce7/admin-ui that the tests didn't look relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any specific examples, but if I were looking I would just be finding another package with a .tsx
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I will try that once I have some more time to work on this. Thanks for the nudge in the right direction.
Added some testing. For the most part, its a copy-paste of the example code from the library's sample code. I ended up finding a few errors on my end, and a few component properties that are not documented correctly in their docs, at least according to their examples. |
@jakebailey, @rbuckton Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
@Weldawadyathink: Everything looks good here. I am ready to merge this PR (at 5befebe) on your behalf whenever you think it's ready. If you'd like that to happen, please post a comment saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ |
Ready to merge |
Please fill in this template.
pnpm test <package to test>
.Select one of these and delete the others:
If adding a new definition:
.d.ts
files generated via--declaration
dts-gen --dt
, not by basing it on an existing project.tsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.