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.

Sign up for GitHub

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

Jump to bottom

converts EuiCodeEditor to TypeScript #2836

Merged
merged 6 commits into from
Feb 19, 2020
Merged

converts EuiCodeEditor to TypeScript #2836

merged 6 commits into from
Feb 19, 2020

Conversation

dimitropoulos
Copy link
Contributor

Summary

closes: #2663

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@dimitropoulos dimitropoulos changed the title Code editor ts converts EuiCodeEditor to TypeScript Feb 7, 2020
@elasticmachine
Copy link
Collaborator

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?

dimitropoulos
dimitropoulos commented Feb 7, 2020
View reviewed changes
scripts/jest/polyfills/mutation_observer.js Outdated
@@ -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
Copy link
Contributor Author

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.

@chandlerprall chandlerprall self-requested a review February 7, 2020 22:35
@chandlerprall chandlerprall self-assigned this Feb 9, 2020
chandlerprall
chandlerprall suggested changes Feb 10, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Couple small pattern things, but also:

src/components/code_editor/index.ts should also export EuiCodeEditorProps

package.json Outdated Show resolved Hide resolved
src/components/code_editor/code_editor.test.tsx Show resolved Hide resolved
src/components/code_editor/code_editor.tsx Show resolved Hide resolved
src/components/code_editor/code_editor.tsx Show resolved Hide resolved
@chandlerprall
Copy link
Contributor

@dimitropoulos just checking in to see if you are still interested in this & the other TypeScript conversions. In addition to reviewing, we are happy to contribute code/changes directly against your PRs, just let us know what works best for you!

@dimitropoulos
Copy link
Contributor Author

definitely! I have a few more things to push, I'll make sure to push them today! Perhaps we can get these merged sooner than later if so?

@chandlerprall
Copy link
Contributor

Thanks! I won't have a chance to review today, but will check out these new changes first thing tomorrow.

@dimitropoulos
Copy link
Contributor Author

thanks! no rush, was just curious. I was away on a business trip the last week or so, hence the silence - so I was excited to get back to these :)

@dimitropoulos dimitropoulos force-pushed the code-editor-ts branch 2 times, most recently from a928368 to ab06cd0 Compare February 19, 2020 19:38
chandlerprall
chandlerprall suggested changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One revert (sorry, again!), plus a final ask and this should be good to go.

src/components/code_editor/code_editor.tsx Show resolved Hide resolved
chandlerprall
chandlerprall reviewed Feb 19, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled & tested locally, including eui.d.ts. Will merge on green ci

@chandlerprall
Copy link
Contributor

jenkins test this

@chandlerprall chandlerprall merged commit 72aa719 into elastic:master Feb 19, 2020
@dimitropoulos dimitropoulos deleted the code-editor-ts branch February 19, 2020 23:42
@dimitropoulos
Copy link
Contributor Author

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! :)

@chandlerprall chandlerprall mentioned this pull request Mar 3, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees

@chandlerprall chandlerprall

Labels
typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EuiCodeEditor needs to be converted to TS
4 participants
@dimitropoulos @elasticmachine @chandlerprall @snide