Skip to content

WIP: [ML] NavMenu conversion to React#39325

Closed
alvarezmelissa87 wants to merge 14 commits intoelastic:masterfrom
alvarezmelissa87:ml-nav-menu-to-react
Closed

WIP: [ML] NavMenu conversion to React#39325
alvarezmelissa87 wants to merge 14 commits intoelastic:masterfrom
alvarezmelissa87:ml-nav-menu-to-react

Conversation

@alvarezmelissa87
Copy link
Contributor

Summary

NavMenu rewrite to React/eui.

Along with the navigation tabs, this PR replaces the kbn_top_nav angular dependency (datepicker and refresh button) with a react TopNav component consisting of the same EuiSuperDatePicker component.

This PR wraps the legacy timefilter in a local class and only pulls the legacy timfilter in that file. All the files that used to pull in the old timefilter now import the new timefilter. This will allow us to easily replace the legacy timefilter when it is replaced in the new platform.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@alvarezmelissa87 alvarezmelissa87 added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.3.0 labels Jun 20, 2019
@alvarezmelissa87 alvarezmelissa87 requested review from a team as code owners June 20, 2019 03:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this function and subscribeToRefreshIntervalUpdate are inconsistent. Maybe just drop the s to give subscribeToUpdate or maybe subscribeToTimeFilterUpdate?

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Jun 20, 2019

Choose a reason for hiding this comment

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

subscribeToUpdates listens for fetch being emitted by the timefilter dependency - this happens both on time update and on refreshInterval update while the subscribeToRefreshIntervalUpdate only listens for the refreshInterval update event. That's why I had the naming that way.

Maybe I can be more explicit? subscribeToTimeAndRefreshIntervalUpdates, subscribeToRefreshIntervalUpdate, maybe if we need to only listen for the time update add a subscribeToTimeUpdate? That way it will be very clear which events are being listened to? Open to ideas on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, in that case I think subscribeToUpdates is fine as it is. Just add a comment clarifying that it happens for both types of update event. Adding a subscribeToTimeUpdate probably worth adding for completeness.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these TODOs still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Has this TODO been done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these needed?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor

lizozom commented Jul 1, 2019

@peteharverson @ppisljar,

While in most apps kbn-top-nav is used to display an options menu (New, Save, Settings, etc.), it seems to me, like in ML (and also Monitoring app) it's used to display content in a tabular fashion.

I think these should be two different components (OptionsMenu and TabbedContent?).

The component I'm working on is used to display options menus (should be ready within a week or so).

I would love to use the work you're doing on displaying tabbed content to create a component that will be offered as part of the arch team react helper components.

I'd love to hear your thoughts on this.

@alvarezmelissa87
Copy link
Contributor Author

Closing in lieu of #40830

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:ml release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v7.4.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants