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

Add loader for kibana overview while checking for user data view #118881

Merged
merged 10 commits into from
Nov 19, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Nov 17, 2021

Summary

Fix #116411
Part of #118866

  • Add a loader while awaiting indexPatternService.hasUserDataView to avoid a blink effect between the data/noData version of the page
  • Use the KibanaThemeProvider for the kibanaOverview application (will be removed from 8.0 and 7.16 backports)
  • Change the label of the 'add data' button to 'Add integrations'
  • Add and adapt unit tests

Checklist

Delete any items that are not applicable to this PR.

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.1.0 labels Nov 17, 2021
Comment on lines +46 to +47
{i18n.translate('kibana-react.kbnOverviewPageHeader.addIntegrationsButtonLabel', {
defaultMessage: 'Add integrations',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component is only used by kibana_overview, but overview_page_footer, in the same parent folder, is also used by other apps, so in doubt, I kept this component in kibana_react.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hoped we can move it to overview, but looks like I missed that overview_page_footer is used in other apps :( #114990

afterAll(() => jest.clearAllMocks());

test('render', async () => {
const component = mountWithIntl(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to switch from shallow to mount because enzyme does not support useEffect with shallow rendering, so all snapshots were in the loading state this PR adds. The snapshots are ugly, but correct.

Comment on lines +137 to +140
const updateComponent = async (component: ReactWrapper) => {
await act(async () => {
await flushPromises();
component.update();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to use that for the indexPatternService.hasUserDataView promise to resolve.

@pgayvallet pgayvallet marked this pull request as ready for review November 18, 2021 10:39
@pgayvallet pgayvallet requested review from a team as code owners November 18, 2021 10:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

}),
RedirectAppLinks: jest.fn((element: JSX.Element) => element),
overviewPageActions: jest.fn().mockReturnValue([]),
OverviewPageFooter: jest.fn().mockReturnValue(<></>),
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: IMO React.createElement(React.Fragment..) is more explicit in this case (and allows removing *.tsx extension)

@@ -148,27 +124,94 @@ const mockFeatures = [
},
];

const flushPromises = async () => {
await new Promise<void>(async (resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

optional

import { setTimeout: setTimeoutP } from 'timers/promises';
const flushPromises = async () => await setTimeoutP(10);

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
kibanaOverview 10.1KB 10.3KB +256.0B

Page load bundle

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

id before after diff
kibanaReact 58.3KB 58.4KB +16.0B

History

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

@pgayvallet pgayvallet merged commit e00de95 into elastic:main Nov 19, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Nov 19, 2021
…stic#118881)

* add loader on overview page

* use KibanaThemeProvider for kibanaOverview

* change add data label to integrations

* adapt tests

* update snapshots, remove i18n keys

* review comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Nov 19, 2021
…stic#118881)

* add loader on overview page

* use KibanaThemeProvider for kibanaOverview

* change add data label to integrations

* adapt tests

* update snapshots, remove i18n keys

* review comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
pgayvallet added a commit that referenced this pull request Nov 19, 2021
#118881) (#119221)

* Add loader for kibana overview while checking for user data view (#118881)

* add loader on overview page

* use KibanaThemeProvider for kibanaOverview

* change add data label to integrations

* adapt tests

* update snapshots, remove i18n keys

* review comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* remove theme provider usage

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
pgayvallet added a commit that referenced this pull request Nov 19, 2021
…ew (#118881) (#119222)

* Add loader for kibana overview while checking for user data view (#118881)

* add loader on overview page

* use KibanaThemeProvider for kibanaOverview

* change add data label to integrations

* adapt tests

* update snapshots, remove i18n keys

* review comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* remove theme provider usage

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Nov 20, 2021
…stic#118881)

* add loader on overview page

* use KibanaThemeProvider for kibanaOverview

* change add data label to integrations

* adapt tests

* update snapshots, remove i18n keys

* review comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
…8881)

* add loader on overview page

* use KibanaThemeProvider for kibanaOverview

* change add data label to integrations

* adapt tests

* update snapshots, remove i18n keys

* review comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
…stic#118881)

* add loader on overview page

* use KibanaThemeProvider for kibanaOverview

* change add data label to integrations

* adapt tests

* update snapshots, remove i18n keys

* review comments

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
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.16.0 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data upload and Add data link under Analytics Tab is inconsistent.
6 participants