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

[ML] Add button for refreshing job list without full page refresh. #24084

Merged
merged 6 commits into from
Oct 17, 2018

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Oct 16, 2018

Summary

Related issue: #17983

New:

Add Refresh button (next to Create New Job) that will refresh job list without a full page refresh.

refreshaligned

Refactor:

Keep jobs.js as a simple display component to pass to the jobsPage directive.

Now, jobs_list_view.js pulls in JobStatsBar, NodeAvailableWarning, and the added EuiButtonEmpty Refresh button.

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, but looking at the screenshot, is it possible to vertically align the text in the Refresh button with the text in the 'Create New Job' button? I wonder if it would look better if the text was aligned across those two buttons?

@walterra
Copy link
Contributor

Just a question about the general structure of the code: Since we have the button for a new job as a separate component NewJobButton, what would you think about also having something like RefreshJobsListButton instead of doing it inline in JobsListView?

@alvarezmelissa87
Copy link
Contributor Author

@peteharverson - great catch! Fixed that and updated the screenshot. 👍

@alvarezmelissa87
Copy link
Contributor Author

Good one, @walterra - I was just thinking that would make it super easy to test, too 👍 I'll go ahead and do that

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Aligned buttons LGTM!

return (
<React.Fragment>
<JobStatsBar
setUpdateJobStats={this.setUpdateJobStats}
Copy link
Member

Choose a reason for hiding this comment

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

couldn't the JobStatsBar now receive the whole jobsSummaryList as a prop and then update itself whenever it changes?
It would remove the need for setUpdateJobStats and unsetUpdateJobStats and make the code a lot simpler.

JobStatsBar could then call createJobStats in getDerivedStateFromProps and create the job stats

/>
<div className="job-management">
<NodeAvailableWarning />
<header>
Copy link
Member

Choose a reason for hiding this comment

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

nit, to keep this cleaner, could this whole header be moved to a component that takes in onRefreshClick and isRefreshing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that but I think it's probably okay to keep each of them separate (the node warning, new button, and refresh button) since they don't share anything and each of them can be tested on their own. 🤔

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alvarezmelissa87
Copy link
Contributor Author

@walterra, @jgowdyelastic - updated from the comments! Would really appreciate if you could take one last look. Thanks! 😄

Copy link
Member

@jgowdyelastic jgowdyelastic 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

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@alvarezmelissa87
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alvarezmelissa87 alvarezmelissa87 merged commit 3602513 into elastic:master Oct 17, 2018
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Oct 18, 2018
…lastic#24084)

* Add refresh button for job list

* Show load icon while refreshing

* Move view build logic to jobsListView

* Align job action buttons in ui

* Extract RefreshJobsListButton component

* update jobStatsBar to take list as prop
alvarezmelissa87 added a commit that referenced this pull request Oct 19, 2018
…24084) (#24219)

* Add refresh button for job list

* Show load icon while refreshing

* Move view build logic to jobsListView

* Align job action buttons in ui

* Extract RefreshJobsListButton component

* update jobStatsBar to take list as prop
@alvarezmelissa87 alvarezmelissa87 deleted the ml-refresh-job-list branch October 19, 2018 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants