-
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
[User Experience App] Improve empty state loading #112531
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.
Looks good, just a few questions.
import { i18n } from '@kbn/i18n'; | ||
import React, { Fragment } from 'react'; | ||
|
||
export function EmptyStateLoading() { |
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.
Should we use one of the loading components instead of a custom component? https://elastic.github.io/eui/#/display/loading
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 the same component, it just adds an EuiEmptyPrompt and a text around it as well.
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 copied this from uptime, we have the same component there.
</PageTemplateComponent> | ||
</CsmSharedContextProvider> | ||
<Fragment> | ||
{isLoading && <EmptyStateLoading />} |
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.
Wondering if we should only be showing this loading state for the UX content, instead of the entire page. Currently we are hiding everything, including the sidebar. I think showing the sidebar can be helpful for the user, especially if the loading is taking a long time and they want to navigate away from the screen easily.
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 loading is only valid for initial hasData call, rest of the loading calls for other content will remain in place.
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 understand, I just wonder if we should be hiding the entire page for this call, instead of just hiding the UX content, since the hasData call is specific to UX content.
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.
got it , i have imoroved it now.
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/uptime (Team:uptime) |
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
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @shahzad31 |
* improve empty state loading * only show loading in content panel * eslint fix
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* improve empty state loading * only show loading in content panel * eslint fix Co-authored-by: Shahzad <[email protected]>
Fixes #112530
Only show empty state if there is no data, improved the UI by removing intermediate empty state showing.
It will show loading as we are fetching the call to detemine has data check.