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

[SharedUX] Fix issue with services package; fix ExitFullScreenButton #127988

Merged
merged 5 commits into from
Mar 18, 2022

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Mar 17, 2022

Summary

This was a fun one.

First, @maksimkovalev and @nreese noticed a bug with ExitFullScreenButton where, when opting to toggleChrome, the chrome wasn't being hidden on mount. So this PR fixes that issue for #126055.

Second, with the recent changes to Shared UX to make it package-based, (#127546) the service context provided by src/plugins/shared_ux has stopped working in x-pack/plugins/maps.

Problem

Digging in, there was a lot of weirdness. A component wrapped with the context within the SharedUX plugin had access to the configured services. But a dependent plugin had a null context.

I messed with it for a few hours and was really starting to doubt my sanity. Talking it through with @spalger this morning, he supplied the answer:

Each plugin that imports a package gets its own copy of that package. That meant Shared UX and Maps were both importing their own copies of the hooks, but those hooks were referencing their own copy of the context.

As a result, providing a wired context as a part of the start contract of the Shared UX plugin meant only the hooks within Shared UX would be referencing the correct Context.

Solution

We have to tweak the API a bit. The Shared UX plugin can provide the configured services as a part of its start contract, but each plugin needs to provide those services to its local version of the SharedUxContextProvider:

import { SharedUxServicesProvider } from '@kbn/shared-ux-services';

public setup(coreSetup: CoreSetup, setupPlugins: MyPluginSetupDeps): MyPluginSetup {
  const [coreStart, startPlugins] = await coreSetup.getStartServices();

  coreSetup.application.register({
    mount: async (params: AppMountParameters) => {
      ReactDOM.render(
        <SharedUxServicesProvider {...startPlugins.sharedUX.getContextServices()}>
          <MyApp />
        </SharedUxServicesProvider>,
        params.element
      );
    }
  );
}

@clintandrewhall clintandrewhall added review loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:critical This issue should be addressed immediately due to a critical level of impact on the product. backport:skip This commit does not require backporting v8.2.0 Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Mar 17, 2022
@clintandrewhall clintandrewhall requested review from a team and spalger March 17, 2022 17:28
@clintandrewhall
Copy link
Contributor Author

@elasticmachine merge upstream

@clintandrewhall clintandrewhall requested a review from spalger March 17, 2022 22:50
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
sharedUX 28 11 -17

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
kibana 309 310 +1

Page load bundle

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

id before after diff
sharedUX 2.4KB 2.0KB -437.0B
Unknown metric groups

API count

id before after diff
sharedUX 6 4 -2

History

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

@clintandrewhall clintandrewhall enabled auto-merge (squash) March 18, 2022 02:29
@clintandrewhall clintandrewhall merged commit 4f6a2fe into main Mar 18, 2022
@clintandrewhall clintandrewhall deleted the shared_ux/packages/services-fix branch March 18, 2022 07:21

// ...wrap React content with the context..
}
```

## Use in Kibana plugins

In order to make consumption of these services easy by Kibana plugins, `src/plugins/shared_ux` provides a pre-wired `SharedUxServicesProvider` component as part of the `start` lifecycle. Plugins can simply make `sharedUX` a dependency and wrap their solution root or any component. See the documentation for `sharedUX` for more details.
In order to make consumption of these services easy by Kibana plugins, `src/plugins/shared_ux` provides a pre-wired set of services as part of the `start` lifecycle. Plugins can simply make `sharedUX` a dependency, import `SharedUsServicesProvider` and wrap their solution root (or any component). See the documentation for `sharedUX` for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Found small typo 😄 SharedUsServicesProvider

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants