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

[shared-ux] Migrate add from library button to shared ux directory #127605

Merged
merged 34 commits into from
Mar 22, 2022

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Mar 14, 2022

Summary

This PR migrates the add from library button from the presentation_utils directory to shared_ux. The primary button has already been migrated in #125998.

  • Solution Toolbar Button Props do not need to be exposed/exported now since this is the only instance of the button props being referenced

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@rshen91 rshen91 changed the title [shared-ux]Migrate add from library button to shared ux directory [shared-ux] Migrate add from library button to shared ux directory Mar 14, 2022
@rshen91 rshen91 added backport:skip This commit does not require backporting loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.2.0 labels Mar 16, 2022
@rshen91 rshen91 marked this pull request as ready for review March 16, 2022 17:39
@rshen91 rshen91 requested a review from a team March 16, 2022 17:39
@rshen91 rshen91 marked this pull request as draft March 16, 2022 21:29
@kibana-ci
Copy link
Collaborator

kibana-ci commented Mar 16, 2022

💔 Build Failed

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/shared-ux-components 2 4 +2
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-components 18 20 +2

History

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

@rshen91 rshen91 self-assigned this Mar 17, 2022
@cchaos
Copy link
Contributor

cchaos commented Mar 17, 2022

TBH, I don't really understand the utility of this button as a "Shared" component. It's simply an instance of the already existing shared component ToolbarButton. Mainly I'm struggling to understand the strategy while converting these to shared-ux components. I think it's worthwhile to reconsider what exists before just doing a straight conversion.

For example, considering that this new AddFromLibraryButton component is actually a very consistently messaged and opinionated component we want folks to re-use, should that just be the component and not have ToolbarButton at all since all that ToolbarButton does is pass props through to an already known component EuiButton.

This may be discussion that needs to happen with the greater team around conversion strategy.

cc @clintandrewhall @majagrubic

@rshen91 rshen91 marked this pull request as ready for review March 17, 2022 19:20
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Left some comments, mostly nits and some cleanup

@rshen91 rshen91 requested a review from majagrubic March 21, 2022 16:26
@rshen91 rshen91 enabled auto-merge (squash) March 22, 2022 14:06
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataViewEditor 130 131 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/shared-ux-components 2 4 +2

Async chunks

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

id before after diff
dataViewEditor 140.2KB 140.2KB +74.0B
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-components 22 24 +2

async chunk count

id before after diff
dataViewEditor 6 5 -1

History

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

cc @rshen91

@rshen91 rshen91 merged commit 7d29236 into elastic:main Mar 22, 2022
@rshen91 rshen91 deleted the migrate-add-from-lib branch March 22, 2022 19:47
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 loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes 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.

4 participants