-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[SecuritySolution] Dashboard listing UI #160540
Conversation
const totalActiveFilters = useMemo( | ||
() => | ||
Object.keys(tagSelection).reduce((acc, currentOption) => { | ||
const inTagReferences = tagReferences?.find((ref) => ref.name === currentOption); | ||
acc += | ||
inTagReferences != null | ||
? tagReferences?.filter((ref) => ref.name === currentOption).length ?? 0 | ||
: 1; | ||
return acc; | ||
}, 0), | ||
[tagReferences, tagSelection] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/content-management/table_list_view_table/src/components/table.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @angorayc
I am sorry but I need to push back changes related to "layout". You've added 3 new props for it:
withPageTemplateHeader
restrictPageSectionWidth
pageSectionPadding
which are all related to changes in layout.
Since #157988 there is a <TableListViewTable />
that is exposed and only contains the Table
which seems to be what you need.
It would probably means that the DashboardListing
should be refactored to only export the Table for Dashboard as well.
If you want we can discuss over Zoom 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments
packages/content-management/table_list_view_table/src/services.tsx
Outdated
Show resolved
Hide resolved
packages/content-management/table_list_view_table/src/services.tsx
Outdated
Show resolved
Hide resolved
packages/content-management/table_list_view_table/src/table_list_view_table.tsx
Outdated
Show resolved
Hide resolved
http, | ||
}, | ||
toMountPoint, | ||
savedObjectsTagging: savedObjectsTagging.hasApi // TODO: clean up this logic once https://github.com/elastic/kibana/issues/140433 is resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to put the changes (useMemo
) I made to fix the tag list issue in Dashboard:
https://github.com/elastic/kibana/pull/160871/files#diff-aecc9bdefcbb1074958f2f7865f975e30c2b6cb24ca907c836df1596a04190ebR209
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start to look good! 👍 One more change from my side to fix a big with the tag list. Cheers
src/plugins/dashboard/public/dashboard_listing/hooks/use_dashboard_listing_table.tsx
Outdated
Show resolved
Hide resolved
src/plugins/dashboard/public/dashboard_listing/hooks/use_dashboard_listing_table.tsx
Show resolved
Hide resolved
src/plugins/dashboard/public/dashboard_listing/dashboard_listing_table.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presentation team changes LGTM! I tested locally and by looking through the code, and everything is working great! Nice work with splitting the Listing up into two components.
One thing that I noticed is that the Security Dashboard listing page has two Create dashboard
buttons - is this expected?
Also left one small nit about the jest testing.
}, | ||
})); | ||
|
||
const mockGetServices = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: some of these services look like stubs which should already be filled out by our stubs in src/plugins/dashboard/jest_setup.ts
. Usually we only override services like this when we need to test a particular behaviour - and in that case it's actually possible to simply override the stub function.
For instance, the default stub for the dashboardSessionStorage.getDashboardIdsWithUnsavedChanges
service defined here: src/plugins/dashboard/public/services/dashboard_session_storage/dashboard_session_storage.stub.ts
returns two dashboards. If you wanted to test what happens if it returns three you could just do
pluginServices.getServices().dashboardSessionStorage.getDashboardIdsWithUnsavedChanges = jest.fn().mockReturnValue([ 'one', 'two', 'three' ]);
In this way we can avoid having to restate all required services for every test. This isn't blocking though, would just help the test be a little less verbose!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an existing create dashboard button on the page and a new one in dashboard listing. I'll leave it as it is at the moment, as users might have used to the position of the existing button.
I've also updated the unit test and re-use the existing mocks. Thanks for the tips @ThomThomson !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍 Thanks for the patience and making those changes @angorayc !
|
||
const savedObjectsTaggingFakePlugin = useMemo( | ||
() => | ||
savedObjectsTagging.hasApi // TODO: clean up this logic once https://github.com/elastic/kibana/issues/140433 is resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue 140433 is closed. Is this comment outdated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's coming from https://github.com/elastic/kibana/pull/160540/files#diff-aecc9bdefcbb1074958f2f7865f975e30c2b6cb24ca907c836df1596a04190ebR62
@ThomThomson Should we remove the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this comment is still valid - the linked issue is just the wrong way to solve it!
src/plugins/dashboard/public/dashboard_listing/dashboard_listing_table.tsx
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/dashboards/pages/landing_page/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/dashboards/pages/landing_page/index.tsx
Outdated
Show resolved
Hide resolved
nit: I noticed it takes some seconds for the Dashboards table to load. Having a loading state/skeleton might be helpful for the user so they know that something is still loading. Jul-20-2023.12-18-42.movI also noticed that the new tag never gets loaded when the know issue happens and you navigate using the menu. Jul-20-2023.12-25-24.movWhen I create a new dashboard it also doesn't show up: new.dashboard.is.hidden.movThe tag count is also wrong to me... Am I doing something wrong? 🤔 |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
@machadoum , thank you so much for testing this PR. I've added a loading icon and implemented the recommended solution of #160723. Please have another try and see if it still happening, thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out once more and it's working splendidly. Thank you, @angorayc!
## Summary 1. Align dashboard listing UI with Kibana dashboard. 2. `Security Solution` tags are selected by default and removable by users. **Prerequisite:** This PR is waiting for elastic#160871 to be merged **Steps to verify:** 1. Visit Security > Dashboards, and create a dashboard from this page. 2. Back to Security Dashboards page, you should see the dashboard you just created and Security Solution tag should be selected by default in the tag filters. 3. Open the tag options, click the Security Solution tag. Observe that it should be removable, and it should display all the dashboards you have in the table. **Known issues:** elastic#160540 (comment) **Before:** <img width="2545" alt="Screenshot 2023-06-27 at 09 24 19" src="https://github.com/elastic/kibana/assets/6295984/bc0fa0b1-96ad-43b0-afc1-48444dfb5691"> **After:** <img width="2543" alt="Screenshot 2023-06-27 at 09 22 21" src="https://github.com/elastic/kibana/assets/6295984/82d0a868-bda2-431f-b0b5-9cbc34d3ae71"> ### Checklist Delete any items that are not applicable to this PR. - [x] 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 --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Pablo Neves Machado <[email protected]>
Summary
Security Solution
tags are selected by default and removable by users.Prerequisite:
This PR is waiting for #160871 to be merged
Steps to verify:
Known issues:
#160540 (comment)
Before:
After:
Checklist
Delete any items that are not applicable to this PR.