-
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
[Papercut][Dashboard][Data Discovery] Add description to saved object finder table if applicable #198816
Conversation
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.
Overall looks good, I left a comment on where to check if there is or not a description for the row item.
@@ -212,6 +213,12 @@ export class SavedObjectFinderUi extends React.Component< | |||
|
|||
public render() { | |||
const { onChoose, savedObjectMetaData } = this.props; | |||
const descriptionsDefined = |
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 think we should make this check separately for each row item. So I would remove it here and change L317 (see comment)
@@ -307,13 +314,23 @@ export class SavedObjectFinderUi extends React.Component< | |||
); | |||
|
|||
const tooltipText = this.props.getTooltipText?.(item); | |||
|
|||
const description = descriptionsDefined && ( |
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.
This could be
const description = !!item.simple.attributes.description && (
<EuiText size="xs" color="subdued" className="eui-textTruncate">
{item.simple.attributes.description}
</EuiText>
)
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.
ooooo TIL boolean !! thanks!
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.
@rshen91 I should have said ask for it in my previous comment: can we update the jest test for the saved object finder to test that description are rendered (if one is provided)? thanks!
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 adding the test! I added a comment about avoiding testing implementation details. I'll approve the PR to unblock you but it would be nice to update the test 👍
@@ -217,6 +219,29 @@ describe('SavedObjectsFinder', () => { | |||
</React.Fragment> | |||
`); | |||
}); | |||
|
|||
it('render description if provided', async () => { |
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.
In this test you are testing implementation details which is not ideal. Changing the implementation (underlying component, props to those component) will break this test.
Concretely you assume that the EuiInMemoryTable
is present and that it takes items
prop.
Now I know that you basically copied what other tests are doing 😊 but let's take this opportunity to improve the test.
In https://github.com/elastic/kibana/blob/main/packages/kbn-test-jest-helpers/src/testbed/testbed.ts I wrote a helper function to parse an EUI table and read its content for testing purpose. Let's copy it over in this file.
So on L37 (below the imports), add the helper func
...
import { coreMock } from '@kbn/core/public/mocks';
// Copied from packages/kbn-test-jest-helpers/src/testbed/testbed.ts
const getTableMetadata = (subject: string, reactWrapper: ReactWrapper) => {
const table = findTestSubject(reactWrapper, subject);
if (!table.length) {
throw new Error(`Eui Table "${subject}" not found.`);
}
const rows = table
.find('tr')
.slice(1) // we remove the first row as it is the table header
.map((row) => ({
reactWrapper: row,
columns: row.find('div.euiTableCellContent').map((col) => ({
reactWrapper: col,
// We can't access the td value with col.text() because
// eui adds an extra div in td on mobile => (.euiTableRowCell__mobileHeader)
value: col.find('div.euiTableCellContent').text(),
})),
}));
// Also output the raw cell values, in the following format: [[td0, td1, td2], [td0, td1, td2]]
const tableCellsValues = rows.map(({ columns }) => columns.map((col) => col.value));
return { rows, tableCellsValues };
};
Now this test can become
it('render description if provided', async () => {
(contentClient.mSearch as any as jest.SpyInstance).mockImplementation(() =>
Promise.resolve({ hits: [doc, doc2, doc4] })
);
const wrapper = mount(
<SavedObjectFinder
services={{ uiSettings, contentClient, savedObjectsTagging }}
savedObjectMetaData={searchMetaData}
/>
);
await nextTick();
wrapper.update();
const table = getTableMetadata('savedObjectsFinderTable', wrapper);
expect(table.tableCellsValues).toEqual([
['Another titleanother description', 'tag-2'],
['Example titleexample description', 'tag-1'],
['Search', 'tag-4'],
]);
});
See how we don't test the underlying implementation. We test that the table renders the correct text.
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.
Nice improvement for the users! LGTM 👍
Let's update the branch and merge? 🙂 |
src/plugins/saved_objects_finder/public/finder/saved_object_finder.test.tsx
Show resolved
Hide resolved
/ci |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
History
cc @rshen91 |
@elasticmachine run docs-build |
Starting backport for target branches: 8.15, 8.16, 8.17, 8.x |
… finder table if applicable (elastic#198816) ## Summary Closes elastic#79754 This PR adds a description under the titles to the saved object finder table that is used in Dashboard's Add to Library Panel. <img width="736" alt="Screenshot 2024-11-07 at 7 55 14 AM" src="https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d"> --------- Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit 23c4306)
… finder table if applicable (elastic#198816) ## Summary Closes elastic#79754 This PR adds a description under the titles to the saved object finder table that is used in Dashboard's Add to Library Panel. <img width="736" alt="Screenshot 2024-11-07 at 7 55 14 AM" src="https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d"> --------- Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit 23c4306)
… finder table if applicable (elastic#198816) ## Summary Closes elastic#79754 This PR adds a description under the titles to the saved object finder table that is used in Dashboard's Add to Library Panel. <img width="736" alt="Screenshot 2024-11-07 at 7 55 14 AM" src="https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d"> --------- Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit 23c4306)
… finder table if applicable (elastic#198816) ## Summary Closes elastic#79754 This PR adds a description under the titles to the saved object finder table that is used in Dashboard's Add to Library Panel. <img width="736" alt="Screenshot 2024-11-07 at 7 55 14 AM" src="https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d"> --------- Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit 23c4306)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…object finder table if applicable (#198816) (#202031) # Backport This will backport the following commits from `main` to `8.x`: - [[Papercut][Dashboard][Data Discovery] Add description to saved object finder table if applicable (#198816)](#198816) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Rachel Shen","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-27T16:38:39Z","message":"[Papercut][Dashboard][Data Discovery] Add description to saved object finder table if applicable (#198816)\n\n## Summary\r\n\r\nCloses https://github.com/elastic/kibana/issues/79754\r\n\r\nThis PR adds a description under the titles to the saved object finder\r\ntable that is used in Dashboard's Add to Library Panel.\r\n\r\n<img width=\"736\" alt=\"Screenshot 2024-11-07 at 7 55 14 AM\"\r\nsrc=\"https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d\">\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"23c4306387725cdafc1c6c6d3aec5049a6f82858","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","v9.0.0","backport:prev-major","papercut"],"title":"[Papercut][Dashboard][Data Discovery] Add description to saved object finder table if applicable","number":198816,"url":"https://github.com/elastic/kibana/pull/198816","mergeCommit":{"message":"[Papercut][Dashboard][Data Discovery] Add description to saved object finder table if applicable (#198816)\n\n## Summary\r\n\r\nCloses https://github.com/elastic/kibana/issues/79754\r\n\r\nThis PR adds a description under the titles to the saved object finder\r\ntable that is used in Dashboard's Add to Library Panel.\r\n\r\n<img width=\"736\" alt=\"Screenshot 2024-11-07 at 7 55 14 AM\"\r\nsrc=\"https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d\">\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"23c4306387725cdafc1c6c6d3aec5049a6f82858"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198816","number":198816,"mergeCommit":{"message":"[Papercut][Dashboard][Data Discovery] Add description to saved object finder table if applicable (#198816)\n\n## Summary\r\n\r\nCloses https://github.com/elastic/kibana/issues/79754\r\n\r\nThis PR adds a description under the titles to the saved object finder\r\ntable that is used in Dashboard's Add to Library Panel.\r\n\r\n<img width=\"736\" alt=\"Screenshot 2024-11-07 at 7 55 14 AM\"\r\nsrc=\"https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d\">\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"23c4306387725cdafc1c6c6d3aec5049a6f82858"}}]}] BACKPORT--> Co-authored-by: Rachel Shen <[email protected]>
… finder table if applicable (elastic#198816) ## Summary Closes elastic#79754 This PR adds a description under the titles to the saved object finder table that is used in Dashboard's Add to Library Panel. <img width="736" alt="Screenshot 2024-11-07 at 7 55 14 AM" src="https://github.com/user-attachments/assets/6a2029cb-1958-4ae3-932b-f7fcb584870d"> --------- Co-authored-by: Davis McPhee <[email protected]>
Summary
Closes #79754
This PR adds a description under the titles to the saved object finder table that is used in Dashboard's Add to Library Panel.