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

Share Modal #179037

Merged
merged 41 commits into from
Apr 4, 2024
Merged

Share Modal #179037

merged 41 commits into from
Apr 4, 2024

Conversation

eokoneyo
Copy link
Contributor

@eokoneyo eokoneyo commented Mar 20, 2024

Summary

This PR is based off of #176617, which in turn builds off of the generic tabbed modal component created here #176261.
Partially addresses https://github.com/elastic/kibana-team/issues/753

The PR introduces an alternate way of sharing a dashboard by consolidating the existing share options into a single place.

TO TEST

Set share.new_version.enabled: true in your kibana.dev.yml in order to see these changes.

@rshen91 rshen91 marked this pull request as ready for review April 2, 2024 17:09
@rshen91 rshen91 requested review from a team as code owners April 2, 2024 17:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@rshen91 rshen91 self-assigned this Apr 2, 2024
@rshen91 rshen91 requested a review from vadimkibana April 2, 2024 17:09
@rshen91 rshen91 added the release_note:skip Skip the PR/issue when compiling release notes label Apr 2, 2024
@rshen91 rshen91 requested review from rshen91 and removed request for rshen91 April 2, 2024 17:27
Copy link
Contributor

@vadimkibana vadimkibana left a comment

Choose a reason for hiding this comment

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

Looks good 👍 ran it locally with the share.new_version.enabled: true flag. It is hidden behind that flag, so we are good to merge to main as the old sharing menu is still the default.


Some small nits:

Not sure why we want the state management to be built into the tabbing modal.

Not necessary to address now, but for the future, in the Storybook stories would nice to have at least on story which has more than one tab, as the main feature this modal provides is tabbing.

Another nit, not to be addressed now, would be nice to break this up into a couple (or so) PRs. For example, the <TabbedModal> could be its own PR.


| Export | Description |
|---|---| |
| `TabbedModal` | component that renders tab content within a modal in kibana.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
| `TabbedModal` | component that renders tab content within a modal in kibana.
| `TabbedModal` | component that renders tab content within a modal in Kibana.

dispatch: Dispatch<IDispatchAction>;
}

const createStateContext = once(<T extends Array<ITabDeclaration<Record<string, any>>>>() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting use of once to make the context creation lazy.

description?: string;
'data-test-subj'?: string;
content: IModalTabContent<S>;
modalActionBtn: IModalTabActionBtn<S>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to make this modalActionBtn optional. I cannot imagine that all tabs will need this.

export interface ITabDeclaration<S = {}> {
id: string;
name: string;
initialState?: Partial<S>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding this initialState, why do we build state management into the modal? What does it solve?

@rshen91 rshen91 enabled auto-merge (squash) April 4, 2024 13:29
@rshen91 rshen91 self-requested a review April 4, 2024 14:15
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Integration Tests #6 / migration v2 migrates saved objects normally when multiple Kibana instances are started at the same time

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
share 85 94 +9

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-tabbed-modal - 8 +8

Async chunks

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

id before after diff
share 6.1KB 3.6KB -2.4KB

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
@kbn/shared-ux-tabbed-modal - 4 +4

Page load bundle

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

id before after diff
share 55.0KB 66.2KB +11.2KB
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-tabbed-modal - 8 +8

References to deprecated APIs

id before after diff
share 6 3 -3

History

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

cc @eokoneyo @rshen91

@rshen91 rshen91 merged commit 03696a4 into elastic:main Apr 4, 2024
37 checks passed
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Apr 4, 2024
rshen91 added a commit that referenced this pull request Apr 4, 2024
## Summary

This splits out the work created in
#179037, so that we might have a
smaller PR to be review that is also single purpose.

This PR introduces the tabbed modal component, alongside visual API for
constructing a modal with tabbed experience within Kibana. It mostly
builds off the EUI modal component, with allowance to construct a modal
declaratively, and also supports managing state for the modal. See
`IModalTabDeclaration`

At the moment this component is quite rudimentary and might be evolved,
but as of now it has mostly been created to support the work for
redesigning the share experience for dashboard, lens and canvas.

It can be reviewed by through the shared_ux storybook; `yarn storybook
shared_ux`. Examples for it's usage have also been included.


## Visuals

<img width="598" alt="Screenshot 2024-03-28 at 12 07 01"
src="https://github.com/elastic/kibana/assets/7893459/830752c8-73c9-435a-94cb-e28442b12941">




Co-authored-by: rshen91 <rshen@elastic.co>
@eokoneyo eokoneyo deleted the rshen-embed-modal-link-logic-rework branch April 11, 2024 11:44
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 release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants