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

Added a SuggestionsTable to Curations view #113123

Merged
merged 4 commits into from
Sep 27, 2021

Conversation

JasonStoltz
Copy link
Member

Summary

This PR implements the Suggestions Table view only. It is not wired up. I will wire it up in a subsequent PR.

screencapture-localhost-5601-pfx-app-enterprise-search-app-search-engines-nationalparks-curations-2021-09-27-10_11_21

Checklist

For maintainers

@JasonStoltz JasonStoltz requested review from a team September 27, 2021 14:12
@JasonStoltz JasonStoltz added v7.16.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed labels Sep 27, 2021
@JasonStoltz
Copy link
Member Author

@elasticmachine merge upstream

}}
onChange={handlePageChange(onPaginate)}
/>
<DataPanel
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using DataPanel for both this new table AND I've updated the curations table to use it as well.

It seems to match exactly what the designs call for and it keep our code consistent.


return (
<DataPanel
icon={<LightbulbIcon />}
Copy link
Member Author

Choose a reason for hiding this comment

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

The lightbulb Icon is not yet part of EUI. For that reason, I created a new component ( thanks to @daveyholler ), and added a new optional icon property to DataPanel that takes an element (as opposed to the existing option that just takes an iconType property).

Comment on lines +77 to +102
// TODO wire up this data
const items: CurationSuggestion[] = [
{
query: 'foo',
updated_at: '2021-07-08T14:35:50Z',
promoted: ['1', '2'],
},
];
const meta = {
page: {
current: 1,
size: 10,
total_results: 100,
total_pages: 10,
},
};
const totalSuggestions = meta.page.total_results;
// TODO
// @ts-ignore
const onPaginate = (...params) => {
// eslint-disable-next-line no-console
console.log('paging...');
// eslint-disable-next-line no-console
console.log(params);
};
const isLoading = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

This entire section is what will be wired up in a subsequent PR.

) : (
<EmptyState />
// TODO
const shouldShowSuggestions = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be wired up in a subsequent PR

Copy link
Contributor

@byronhulcher byronhulcher left a comment

Choose a reason for hiding this comment

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

This looks good but small feedback on adding the icon prop to the DataPanel component.

@@ -30,6 +30,7 @@ interface Props {
titleSize?: EuiTitleProps['size'];
subtitle?: React.ReactNode;
iconType?: EuiIconProps['type'];
icon?: React.ReactElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t think you actually need to add an icon prop to DataPanel . The type property of an EuiIcon component already accepts a ReactElement, for instance here we’re already using that functionality to pass a custom SVG icon into a DataPanel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh nice! I had no idea, I appreciate that. I will make the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@byronhulcher byronhulcher left a comment

Choose a reason for hiding this comment

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

Thanks for making that change, let's merge!

@JasonStoltz JasonStoltz enabled auto-merge (squash) September 27, 2021 19:09
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1446 1449 +3

Async chunks

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

id before after diff
enterpriseSearch 1.3MB 1.3MB +3.9KB

History

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

cc @byronhulcher

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 27, 2021
Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants