[Presentation] Deprecate Solution Toolbar Button in favor of Shared UX Component #126916
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rshen91
added
backport:skip
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
ThomThomson
approved these changes
Mar 4, 2022
There was a problem hiding this comment.
Change LGTM!
I'm wondering though, since it seems like this component is only used in Presentation Util itself, it could be quite trivial to migrate to the version in shared UX immediately, and remove the version in presentation_util.
Is there a reason to leave it deprecated for a while? Or should we think about removing it ASAP?
…n' into deprecate-solution-toolbar-button
mdefazio
approved these changes
Mar 9, 2022
…n' into deprecate-solution-toolbar-button
6 tasks
majagrubic
reviewed
Mar 9, 2022
| "embeddable", | ||
| "expressions", | ||
| "dataViews", | ||
| "sharedUx" |
There was a problem hiding this comment.
The plugin's name is sharedUX; I think that's why the build went crazy.
majagrubic
reviewed
Mar 9, 2022
src/plugins/presentation_util/public/components/solution_toolbar/items/popover.tsx
Outdated
Show resolved
Hide resolved
majagrubic
reviewed
Mar 9, 2022
| @@ -25,6 +25,12 @@ export const LazySolutionToolbarButton = React.lazy(() => | |||
| })) | |||
| ); | |||
|
|
|||
| export const LazySolutionToolbarButtonProps = React.lazy(() => | |||
| import('./toolbar/index').then(({ SolutionToolbarButtonProps }: any) => ({ | |||
There was a problem hiding this comment.
Two things: first, this should exist.
LazySolutionToolbarButtonProps is a Typescript interface, so there's no lazy load necessary-- types are removed at compile time.
Second, as @majagrubic pointed out: no need to tack /index onto this import.
| @@ -39,6 +45,8 @@ export const ExitFullScreenButton = withSuspense(LazyExitFullScreenButton); | |||
| */ | |||
| export const SolutionToolbarButton = withSuspense(LazySolutionToolbarButton); | |||
|
|
|||
| export const SolutionToolbarButtonProps = withSuspense(LazySolutionToolbarButtonProps); | |||
There was a problem hiding this comment.
Suggested change
| export const SolutionToolbarButtonProps = withSuspense(LazySolutionToolbarButtonProps); |
src/plugins/presentation_util/public/components/solution_toolbar/solution_toolbar.tsx
Outdated
Show resolved
Hide resolved
…n' into deprecate-solution-toolbar-button
spalger
reviewed
Mar 10, 2022
| @@ -18,7 +18,7 @@ | |||
| "embeddable", | |||
| "expressions", | |||
| "dataViews", | |||
| "sharedUx" | |||
| "sharedUX" | |||
There was a problem hiding this comment.
For plugin ids we have historically kept to the most simple form of camelcase, and only capitalized the first letter in an abbreviation. Said another way, the capital just replaces the _ in the snake_case version of the word, so shared_ux would be come sharedUx
There was a problem hiding this comment.
Oh, I was confused about which PR this is, maybe @clintandrewhall is open to a rename PR outside of this work or maybe when the number of inflight PRs is a bit lower.
…n' into deprecate-solution-toolbar-button
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Related to #125998
The solution toolbar button was moved to the Shared UX team. This PR deprecates the solution toolbar button for the presentation team so that both components do not have to be maintained.