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

refactor(mui-datatables): updated for v3.1.x #45306

Merged
merged 2 commits into from
Jul 7, 2020
Merged

refactor(mui-datatables): updated for v3.1.x #45306

merged 2 commits into from
Jul 7, 2020

Conversation

favna
Copy link
Contributor

@favna favna commented Jun 5, 2020

  1. Noticed a missing key on textLabels.body, this PR adds it
  2. Updated the package to support mui-datatables version 3.1.x
  3. Also covered the changes to resolve Include debounce search render plugin #45264 - no idea what OP there meant with "when I find more cycles", but after @danger-public alerted me of the missing property I figured I should add it.

Please fill in this template.

@typescript-bot typescript-bot added Where is GH Actions? GH Actions didn't give a response to this PR Author is Owner The author of this PR is a listed owner of the package. labels Jun 5, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 5, 2020

@favna Thank you for submitting this PR!

This is a live comment which I will keep updated.

Code Reviews

Because this PR edits the configuration file, it can be merged once it's reviewed by a DT maintainer.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ❌ A DT maintainer needs to merge changes which affect module config files

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 45306,
  "author": "Favna",
  "owners": [
    "favna",
    "ankithkonda",
    "hwatersiv",
    "souppower",
    "byrekt"
  ],
  "dangerLevel": "ScopedAndConfiguration",
  "headCommitAbbrOid": "3d028d0",
  "headCommitOid": "3d028d0d76c07f735296344ac0cbeb34bcbfe0db",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastCommitDate": "2020-07-06T22:49:06.000Z",
  "reopenedDate": "2020-06-12T22:54:26.000Z",
  "lastCommentDate": "2020-07-07T18:23:31.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45306/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "anyPackageIsNew": false,
  "packages": [
    "mui-datatables"
  ],
  "files": [
    {
      "filePath": "types/mui-datatables/index.d.ts",
      "kind": "definition",
      "package": "mui-datatables"
    },
    {
      "filePath": "types/mui-datatables/mui-datatables-tests.tsx",
      "kind": "test",
      "package": "mui-datatables"
    },
    {
      "filePath": "types/mui-datatables/tsconfig.json",
      "kind": "package-meta",
      "package": "mui-datatables"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-07-06T23:35:23.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 1,
  "isChangesRequested": false
}

@typescript-bot
Copy link
Contributor

🔔 @ankithkonda @hwatersiv @souppower @byrekt - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Jun 5, 2020
@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊
master #45306 diff
Batch compilation
Memory usage (MiB) 139.3 121.8 -12.6%
Type count 23792 23796 0%
Assignability cache size 68892 68896 0%
Language service
Samples taken 236 242 +3%
Identifiers in tests 236 242 +3%
getCompletionsAtPosition
    Mean duration (ms) 508.5 504.4 -0.8%
    Mean CV 8.8% 9.3%
    Worst duration (ms) 670.9 628.6 -6.3%
    Worst identifier button span
getQuickInfoAtPosition
    Mean duration (ms) 505.0 500.0 -1.0%
    Mean CV 8.9% 9.4% +6.0%
    Worst duration (ms) 632.2 578.7 -8.5%
    Worst identifier span event

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jun 5, 2020
@favna favna marked this pull request as draft June 11, 2020 13:19
@favna
Copy link
Contributor Author

favna commented Jun 11, 2020

Drafting this so I can include the changes made in mui-datatables v3.0 before it gets merged. That way it's all inclusive.

Changes to cover: https://github.com/gregnb/mui-datatables/blob/master/docs/v2_to_v3_guide.md

@favna favna changed the title refactor(mui-datatables): added extra key to textLabels refactor(mui-datatables): updated for v3.0.0 Jun 12, 2020
@favna favna marked this pull request as ready for review June 12, 2020 22:54
@favna
Copy link
Contributor Author

favna commented Jun 12, 2020

Undrafted, PR now covers the changes for version 3.0.0 as well.

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jun 12, 2020
@typescript-bot
Copy link
Contributor

@favna The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Jun 13, 2020
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jun 13, 2020
@typescript-bot
Copy link
Contributor

@favna The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Jun 13, 2020
@favna
Copy link
Contributor Author

favna commented Jun 13, 2020

Since this PR was drafted then undrafted it seems like it got reset for the 7 day turn around cycle, seeing its status on the project board. Just going to ping the other maintainers in here like @typescript-bot does for review so this can get merged:

🔔 @ankithkonda @hwatersiv @souppower @byrekt - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot typescript-bot added the Check Config Changes a module config files label Jun 24, 2020
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jul 6, 2020
@typescript-bot
Copy link
Contributor

@favna The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@peterblazejewicz
Copy link
Member

@favna
[optional]
if you rebase your branch on master, it should cleanly build, now the diff checker detects outer changes. Tested locally:

First, rewinding head to replay your work on top of it...
Applying: refactor(mui-datatables): added extra key to textLabels
Applying: refactor(mui-datatables): update mui-datatables to v3.0.0
Applying: refactor(mui-datatables): include debounce search render plugin
Applying: address peter's review
Applying: add missing types since 3.0.x

(so it rebases cleanly)
and diffs for tests:

Running: git diff master --name-status
M	types/mui-datatables/index.d.ts
M	types/mui-datatables/mui-datatables-tests.tsx
M	types/mui-datatables/tsconfig.json
Testing 1 changed packages: mui-datatables
Testing 0 dependent packages: 
Installing NPM dependencies...

Copy link
Member

@peterblazejewicz peterblazejewicz left a comment

Choose a reason for hiding this comment

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

LGTM!
I've tried samples from v3.* README

@favna thx for the update!

@typescript-bot typescript-bot added Other Approved This PR was reviewed and signed-off by a community member. and removed Other Approved This PR was reviewed and signed-off by a community member. The CI failed When GH Actions fails labels Jul 6, 2020
@favna
Copy link
Contributor Author

favna commented Jul 6, 2020

@peterblazejewicz rebased on master and squashed

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Jul 6, 2020
@typescript-bot
Copy link
Contributor

@favna The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Jul 6, 2020
@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Jul 6, 2020
@occult
Copy link

occult commented Jul 7, 2020

Thank you so much! It will help out a lot. LGTM :)

@alycoy
Copy link

alycoy commented Jul 7, 2020

LGTM!

@weswigham weswigham merged commit f09343c into DefinitelyTyped:master Jul 7, 2020
@typescript-bot
Copy link
Contributor

I just published @types/mui-datatables@3.1.0 to npm.

@favna favna deleted the refactor/mui-datatables/add-missing-text-key branch July 7, 2020 19:29
@peterblazejewicz
Copy link
Member

👯

@peterblazejewicz
Copy link
Member

peterblazejewicz commented Jul 7, 2020

fixed #45909

ngbrown pushed a commit to ngbrown-forks/DefinitelyTyped that referenced this pull request Jul 11, 2020
* refactor(mui-datatables): updated for v3.1.x

* fix stuff
danielrearden pushed a commit to danielrearden/DefinitelyTyped that referenced this pull request Sep 22, 2020
* refactor(mui-datatables): updated for v3.1.x

* fix stuff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author is Owner The author of this PR is a listed owner of the package. Check Config Changes a module config files Other Approved This PR was reviewed and signed-off by a community member. Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants