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] Ensure loading indicator is present on initial jobs load #27151

Merged

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Dec 13, 2018

Summary

Related issue: #26418

  • Adds loading to JobsListView state with initial null value.
  • If loading set to null, it is set to true when loading job list - this ensures loading indicator is shown on first page load only.
  • JobsListUI (containing the job list table) receives loading as prop.

Checklist

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

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@alvarezmelissa87 alvarezmelissa87 added review enhancement New value added to drive a business result :ml Feature:Anomaly Detection ML anomaly detection labels Dec 13, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@alvarezmelissa87
Copy link
Contributor Author

alvarezmelissa87 commented Dec 13, 2018

I noticed that the .catch in jobs_list_view.js#refreshJobSummaryList doesn't actually prevent the page from going full broken. Refactoring it to use async/await and wrapping the call in a try/catch block will actually catch the error.

Does it feel okay to do that refactor in this PR or do it separately?
cc @peteharverson, @walterra, @jgowdyelastic

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alvarezmelissa87
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jgowdyelastic
Copy link
Member

@alvarezmelissa87 i'm fine with that refactor happening in this PR. it won't be big.

@peteharverson
Copy link
Contributor

@alvarezmelissa87 refactoring refreshJobSummaryList in this PR to use async/await seems a good idea to me. No need for a separate PR

@alvarezmelissa87
Copy link
Contributor Author

This has been updated with the refreshJobSummaryList async/await refactor. Would you be up for taking another look?

@elasticmachine
Copy link
Contributor

💔 Build Failed

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.

LGTM

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 Succeeded

@alvarezmelissa87 alvarezmelissa87 merged commit a88778b into elastic:master Dec 14, 2018
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Dec 14, 2018
…c#27151)

* Show table loading on initial jobs load

* Use async/await and try/catch to catch job load failure
@alvarezmelissa87 alvarezmelissa87 deleted the ml-job-list-loading-indicator branch December 14, 2018 17:14
alvarezmelissa87 added a commit that referenced this pull request Dec 14, 2018
#27231)

* Show table loading on initial jobs load

* Use async/await and try/catch to catch job load failure
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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Anomaly Detection ML anomaly detection :ml review v6.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants