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] Update the Overview page #159609

Merged
merged 24 commits into from
Jun 20, 2023
Merged

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Jun 13, 2023

Summary

Resolves #154294 and updates the UI of the Overview page

  • Updates panels layout
  • Stores expand/collapsed state of the panels in the local storage
  • Update empty states text and layout
image

Checklist

@darnautov darnautov self-assigned this Jun 13, 2023
@darnautov darnautov requested a review from a team as a code owner June 13, 2023 16:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@darnautov darnautov requested a review from szabosteve June 13, 2023 16:45
@@ -43,3 +43,7 @@ export function lazyMlNodesAvailable() {
export function permissionToViewMlNodeCount() {
return userHasPermissionToViewMlNodeCount;
}

export function getMlNodesCount(): number {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we should create a service to retrieve and store ML nodes data, but it'd be too much for this PR

@@ -158,5 +164,14 @@ export function getStatsBarData(jobsList: any) {

jobStats.activeNodes.value = Object.keys(mlNodes).length;

if (jobStats.total.value === 0) {
for (const statKey in jobStats) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Object.entries here instead?

    for (const [statKey, val] of Object.entries(jobStats)) {
      if (statKey !== 'total') {
        val.show = false;
      }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 0380da1

@qn895
Copy link
Member

qn895 commented Jun 13, 2023

Probably not related to this PR, but noticed 'Updating' status stays for a while (sometimes close to a minute) when navigating from the search bar at the top. Refreshing the page or using the button is pretty speedy.

Screen.Recording.2023-06-13.at.15.11.10.mov

Also out of scope of this PR, but I think it would be nice to match the stats bar in the AD/DFA pages:
Screen Shot 2023-06-13 at 15 20 12
Screen Shot 2023-06-13 at 15 20 21

@darnautov
Copy link
Contributor Author

Probably not related to this PR, but noticed 'Updating' status stays for a while (sometimes close to a minute) when navigating from the search bar at the top. Refreshing the page or using the button is pretty speedy.

Screen.Recording.2023-06-13.at.15.11.10.mov
Also out of scope of this PR, but I think it would be nice to match the stats bar in the AD/DFA pages: Screen Shot 2023-06-13 at 15 20 12 Screen Shot 2023-06-13 at 15 20 21

@qn895 I didn't manage to reproduce, unfortunately. Could it be some issues with the dev server? Otherwise can you please provide step-by-step instructions?

const getAnalytics = getAnalyticsFactory(
setAnalytics,
setAnalyticsStats,
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Could this be fixed? If not can it have a comment.
We should probably use ts-expect-error too, so it's obvious when the issue it fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored in 4d2ded2

if (jobStats.total.value === 0) {
for (const statKey in jobStats) {
if (statKey !== 'total') {
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

same with this @ts-ignore

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 one was removed in 0380da1

const { euiTheme } = useCurrentThemeVars();

return (
<EuiSplitPanel.Outer grow hasShadow={false} hasBorder={isOpen}>
Copy link
Member

Choose a reason for hiding this comment

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

nit, if you expand and collapse these sections they resize by 1px because of the lack of border.
Adding a border when they are collapsed looks a bit nicer and is less flickery.
Something like this, euiColorLightestShade is the closest shade I could find.

    css={isOpen ? {} : { border: `1px solid ${euiTheme.euiColorLightestShade}` }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f846d55

Copy link
Member

Choose a reason for hiding this comment

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

I'm being very nitty now :D
The problem with using transparent rather than matching the color of the background means that the panels are all 1px smaller all the way around when collapsed. This is obvious to me when using a low res screen.
But normal people might not care.

image

image

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

I left some suggestions for the UI text.

…s/analytics_management/components/empty_prompt/empty_prompt.tsx

Co-authored-by: István Zoltán Szabó <[email protected]>
Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

UI text with the latest changes LGTM! Thank you!

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
Please ignore my comment about the panel sizes, it's something that we can tweak alter if we need to.

@qn895
Copy link
Member

qn895 commented Jun 14, 2023

LGTM 🎉

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.

With the changes to the links in the empty states, please could you unskip the ml and transform accessibility tests in https://github.com/elastic/kibana/blob/main/x-pack/test/accessibility/config.ts#L38 and check if they are now passing.

@bhavyarm can you remember why the transform plugin accessibility tests were skipped in #144940? The two PRs referenced in that PR don't seem to be related to the transforms page.

@bhavyarm
Copy link
Contributor

@bhavyarm can you remember why the transform plugin accessibility tests were skipped in #144940? The two PRs referenced in that PR don't seem to be related to the transforms page.

@peteharverson transforms failed in the ci run at one point. But yes they were passing in the local.

Once a11y tests are unskipped - is it possible to run this PR in flaky test runner 100 times? Thanks very much!

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 - hoping the accessibility tests pass now! If not keep them skipped and we'll look at in a follow up.

@darnautov
Copy link
Contributor Author

darnautov commented Jun 20, 2023

Flaky tests runner for accessibility tests https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2412

Update: several failures in a11y tests are not related to this PR, e.g., anomaly detection create advanced job validation step and transform tests. I'll keep them disabled in this PR.

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1741 1751 +10

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.4MB 3.4MB +8.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 73.9KB 73.9KB +56.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 411 415 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 494 498 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @darnautov

@darnautov darnautov merged commit 6ac52fb into elastic:main Jun 20, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 20, 2023
@darnautov darnautov deleted the ml-154294-overview-page branch June 20, 2023 11:56
mistic added a commit that referenced this pull request Jun 20, 2023
@mistic
Copy link
Member

mistic commented Jun 20, 2023

I had the need to revert this PR as it was failing the on merge ci https://buildkite.com/elastic/kibana-on-merge/builds/31675

darnautov added a commit to darnautov/kibana that referenced this pull request Jun 20, 2023
## Summary

Resolves elastic#154294 and updates the
UI of the Overview page

- Updates panels layout
- Stores expand/collapsed state of the panels in the local storage
- Update empty states text and layout

<img width="1341" alt="image"
src="https://github.com/elastic/kibana/assets/5236598/8833fa2a-b574-44ee-bacb-e974186dd35f">

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

(cherry picked from commit 6ac52fb)
darnautov added a commit that referenced this pull request Jun 20, 2023
## Summary


[Cherry-pick](36c9a0c)
of the original PR #159609 with
the
[fixed](d444ca4)
`css` import.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting :ml release_note:enhancement reverted Team:ML Team label for ML (also use :ml) v8.9.0
Projects
None yet
10 participants