-
Notifications
You must be signed in to change notification settings - Fork 842
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
converts EuiCode and EuiCodeBlock to TypeScript #2835
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
8685c73
to
2310a4f
Compare
@@ -524,7 +524,7 @@ var MutationNotifier = /** @class */ (function (_super) { | |||
__extends(MutationNotifier, _super); | |||
function MutationNotifier() { | |||
var _this = _super.call(this) || this; | |||
_this.setMaxListeners(150); // bump this as needed - some tests do not perform the unmounting lifecycle | |||
_this.setMaxListeners(294); // bump this as needed - some tests do not perform the unmounting lifecycle |
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 is suspicious to me, but it's all that worked to get past the pre-commit hook. Any deeper insight is appreciated.
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.
Definitely agree, I'll take a deeper look at this.
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.
note: at the time I made this PR this change was necessary on master as well
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.
Very interesting; can you provide some details on your setup:
- operating system & version
- yarn version (and confirm you used
yarn
to install EUI's dependencies, notnpm
) - confirm the
jest
package used comes fromeui/node_modules/jest
and not a global install
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.
λ $ uname -a
Linux dimitri 5.3.0-26-generic #28-Ubuntu SMP Wed Dec 18 05:37:46 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
λ $ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 19.10
Release: 19.10
Codename: eoan
λ $ yarn --version
1.21.1
λ $ jest
Command 'jest' not found, did you mean:
command 'test' from deb coreutils (8.30-3ubuntu2)
Try: sudo apt install <deb name>
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.
would you like me to remove the commit where I upped the count or shall we just leave it in?
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.
Thanks for putting this together, @dimitropoulos!
Left a couple comments - (almost?) everything is small to align better with our patterns (which, as you observed, aren't always well followed to begin with and aren't documented [which is coming...])
After these, I'll take a second pass on some of those shared prop definitions along with verifying backwards compatibility.
04fceba
to
136cc4a
Compare
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.
Two additional things on the exported types, otherwise this looks great
23bcef8
to
3c0a28f
Compare
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.
One final change requested; everything else looks good, pulled & tested locally, built eui.d.ts and confirmed the expected values are available from the @elastic/eui
package space
636be6d
to
15cbc88
Compare
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.
Changes LGTM! Thank you very much for investing the time and effort on this @dimitropoulos , we really appreciate it!
jenkins test this will merge on green ci |
My pleasure! |
hey and by the way @chandlerprall (or anyone else) if anything comes up regarding improvements or bugs for this in the future feel free to ping me - I can't promise I will have time to help but I can promise I'll try! :) |
Summary
closes #2665
Checklist