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

turning it all into one React app #35185

Merged
merged 16 commits into from
Apr 19, 2019

Conversation

bmcconaghy
Copy link
Contributor

This deletes a lot of Angular code and turns it all into one big React app. I still need to address breadcrumbs and reducing the refetching of watches, but those will get addressed in a separate PR.

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

@bmcconaghy this is awesome!

i didn't test locally yet. it looks like there are a couple files that may have been deleted accidentally.

  • confirm_watches_modal.tsx
  • form_errors.tsx
  • delete_watches_modal.tsx

@@ -0,0 +1,3 @@
<kbn-management-app section="elasticsearch/watcher">
<div id="watchReactRoot"></div>
</kbn-management-app>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no new line

@@ -0,0 +1,23 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

for my own knowledge... do we still need this now that we switched to react router?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

arguably not. probably should be using history for nav.

{selectedTab === WATCH_EDIT_TAB && (
<JsonWatchEditForm urlService={urlService} licenseService={licenseService} />
)}
{selectedTab === WATCH_EDIT_TAB && <JsonWatchEditForm licenseService={licenseService} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than passing this in as a prop, could you just import LicenseServiceContext directly from the JsonWatchEditForm component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, fixed.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@bmcconaghy
Copy link
Contributor Author

@alisonelizabeth thx for the comments, pushed fixes for those things.

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@bmcconaghy
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@bmcconaghy
Copy link
Contributor Author

retest

@bmcconaghy
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@bmcconaghy bmcconaghy merged commit cadca11 into elastic:watcher-port Apr 19, 2019
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.

3 participants