Skip to content

[Enterprise Search] Search indices empty states#131337

Merged
efegurkan merged 7 commits intoelastic:mainfrom
efegurkan:Search-Indices-Empty-States
May 3, 2022
Merged

[Enterprise Search] Search indices empty states#131337
efegurkan merged 7 commits intoelastic:mainfrom
efegurkan:Search-Indices-Empty-States

Conversation

@efegurkan
Copy link
Copy Markdown
Member

@efegurkan efegurkan commented May 2, 2022

Closes https://github.com/elastic/enterprise-search-team/issues/1938

Summary

Screenshot 2022-05-02 at 18 10 01
Screenshot 2022-05-02 at 17 17 38
Screenshot 2022-05-02 at 18 10 32

Binds Empty states to Search Indices route. Backend calls will be added later, for now some static data added to the logic files. Actions are exposed to the window under contentActions for testing, to be removed when Backend calls added.

Checklist

Delete any items that are not applicable to this PR.

@efegurkan efegurkan added release_note:skip Skip the PR/issue when compiling release notes Team:EnterpriseSearch v8.3.0 labels May 2, 2022
@efegurkan efegurkan requested a review from a team May 2, 2022 15:25
@efegurkan
Copy link
Copy Markdown
Member Author

@elasticmachine merge upstream

@efegurkan efegurkan changed the title Search indices empty states [Enterprise Search] Search indices empty states May 2, 2022
const { searchIndices, searchEngines } = useValues(SearchIndicesLogic);

useEffect(() => {
loadSearchIndices();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick(non-blocking): I generally prefer using a single InitPageAction that triggers these data-load actions, rather than having two separate effects that trigger specific data fetches. It's more easily maintainable going forward, and makes re-use of and preventing unnecessary API calls easier.

<>
<EnterpriseSearchContentPageTemplate
pageChrome={baseBreadcrumbs}
pageViewTelemetry="Search indices"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: do we have schema's or a strategy defined for this telemetry somewhere? I've seen bits and pieces, but no comprehensive document.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am not aware of it if there are any 🤷

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a thing we should work on getting then :)

});

it('has expected default values', () => {
mount();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: you can move this mount to beforeEach and remove it from the specific tests.

path: ['enterprise_search', 'content', 'search_indices', 'search_indices_logic'],
actions: {
loadSearchIndices: true,
onSearchIndicesLoad: (searchIndices) => searchIndices,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: I'm not a fan of this onXXX syntax in Redux/Kea actions. Redux actions are messages in a messaging bus, they are not really functions that happen and produce a result. Rather, the reducer and listener process these messages to create a new state and/or make things happen.

The message you're sending is not that 'onLoad' is happening, it's really that the API call succeeded--so I would usually call these actions something like searchIndicesLoadSuccess. I'd also consider adding a searchIndicesLoadError action to be able to handle the error separately from the API call--makes form handling much easier, for instance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will change them to searchWhateverSuccess since we didn't define the failure cases yet, will skip that, but definitely a nice convention to follow, I like the idea 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a big fan of this pattern, we should document how we build logic files for Enterprise Search

},
listeners: ({ actions }) => ({
initPage: async () => {
try {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: you should be able to remove this try/catch block, as dispatching these actions should ~never error

@efegurkan efegurkan enabled auto-merge (squash) May 3, 2022 13:57
@kibana-ci
Copy link
Copy Markdown

💛 Build succeeded, but was flaky

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1462 1480 +18

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.5MB 1.5MB +14.5KB

Page load bundle

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

id before after diff
enterpriseSearch 20.6KB 20.6KB +14.0B
Unknown metric groups

async chunk count

id before after diff
enterpriseSearch 8 9 +1

History

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

@efegurkan efegurkan merged commit 94753e6 into elastic:main May 3, 2022
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label May 3, 2022
@efegurkan efegurkan deleted the Search-Indices-Empty-States branch May 4, 2022 11:08
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
* Bind Search Indices empty state with Mock data
* Add Logic tests
* Change title on empty state to match with designs
* Hide title correctly on empty state
* Review changes


Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:EnterpriseSearch v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants