[shared-ux] Migrate redirect app links component #124197

rshen91 · 2022-02-01T02:26:59Z

Summary

This PR encompasses the migration of the redirect app link component into the shared_ux directory.

Where the component is used currently:
- workspace_panel.tsx in lens
- add_data.tsx in public/application/components/add_data/add_data.tsx
- manage_data.tsx in public/application/components/manage_data/manage_data.tsx
- renderAppCard and <KibanaPageTemplate /> in overview.tsx in plugins/kibana_overview/public/components/overview/overview.tsx
- actionAddData in overview_page_actions.tsx in plugins/kibana_react/public/overview_page/overview_page_actions/overview_page_actions.tsx
- defaultRouteButton in overview_page_footer.tsx in plugins/kibana_react/public/overview_page/overview_page_footer
- Elastic Agent Card in elastic_agent_card.tsx in kibana_react/public/page_template/no_data_page/no_data_card
- Saved Object Edition page in saved_objects_edition_page.tsx in saved_objects_management/public/management_section/saved_objects_edition_page.tsx
- Saved Objects Table in saved_objects_table.tsx in plugins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
- getTableColumns in get_table_columns.tsx in plugins/visualizations/public/visualize_app/utils/get_table_columns.tsx
- Tests --> HomePage in app.tsx test/plugin_functional/plugins/core_history_block/plubic/app.tsx
- exploratory_view_example in mount.tsx in x-pack/examples/exploratory_view_example/public/mount.tsx
- UXAppRoot in ux_app.tsx in x-pack/plugins/apm/public/application/ux_app.tsx
- ApmAppRoot in app_root.tsx in x-pack/plugins/apm/public/components/routing/app_root.tsx
- getColumns in get_columns.tsx in x-pack/plugins/data_enhanced/public/search/sessions_mgmt/lib/get_columns.tsx
- createConnectedSearchSessionIndicator in connected_search_session_indicator.tsx
- FleetAppContext in x-pack/plugins/fleet/public/applications/fleet/app.tsx
- AgentLogsUI in agent_logs.tsx in x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_details_page/components/agent_logs/agent_logs.tsx
- IntegrationsAppContext in app.tsx in x-pack/plugins/fleet/public/applications/integrations/app.tsx
- CustomAssetsAccordion in custom_assets_accordion.tsx in x-pack/plugins/fleet/public/components/custom_assets_accordion.tsx
- TutorialDirectoryHeaderLink in tutorial_directory_header_link.tsx x-pack/plugins/fleet/public/components/home_integration/tutorial_directory_header_link.tsx
- Index Lifecycle Management in index.tsx in x-pack/plugins/index_lifecycle_management/public/application/index.tsx
- Infrastructure Page in index.tsx x-pack/plugins/infra/public/pages/metrics/index.tsx
- VisualizationWrapper in workspace_panel.tsx x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx
- CommonPageWrapper in ml_page.tsx x-pack/plugins/ml/public/application/components/ml_page/ml_page.tsx
- Jobs List Page in jobs_list_page.tsx x-pack/plugins/ml/public/application/management/jobs_list/components/jobs_list_page/jobs_list_page.tsx
- Observability in index.tsx x-pack/plugins/observability/public/application/index.tsx
- Space Management App in x-pack/plugins/spaces/public/management/spaces_management_app.tsx
- AppHandlingClusterUpgradeState in x-pack/plugins/upgrade_assistant/public/application/app.tsx
- Application in x-pack/plugins/uptime/public/apps/uptime_app.tsx
- Alert Messages in x-pack/plugins/uptime/public/lib/alert_types/alert_messages.tsx

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

rshen91 · 2022-02-03T14:24:47Z

@elasticmachine merge upstream

majagrubic · 2022-03-07T16:16:18Z

I pushed a change that adds useContainerRef as a prop to the inner component. But I would strongly advise against this inner component; I see little value in a component that's basically just a div, and it increases complexity.
Another thing I noticed - the naming. The file is called redirect_app_link; the component itself is called RedirectAppLinkS. I understand this was taken from kibana_react, but since we're already refactoring the component, I suggest we make the naming consistent.

spalger

@stacey-gammon The changes in packages/kbn-docs-utils/src/api_docs/tests/snapshots/ are triggering operations review, they look fine to me but you're probably the best one to make sure those are valid changes.

src/plugins/shared_ux/public/components/app_links/click_handler.ts

src/plugins/shared_ux/public/components/app_links/utils.ts

…app_links

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.stories.tsx

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.tsx

src/plugins/shared_ux/public/components/redirect_app_links/utils.ts

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.tsx

cchaos · 2022-03-08T15:06:53Z

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.stories.tsx

+          navigateToUrl={() => Promise.resolve()}
+          currentAppId$={new BehaviorSubject('test')}
+        >
+          <EuiButton data-test-subj="homeAddData" fill iconType="plusInCircle">


Does this button not need an href for the behavior to be picked up?

Maybe there's also a way to output something so we can see that it is working? I think @majagrubic did something like that for the No Data Views component under Storybook's Action tab.

Added an onClick handler in the story - let me know if this is what you meant. Thanks!
d1fd102

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.stories.tsx

clintandrewhall · 2022-03-09T20:25:46Z

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.tsx

+ *
+ * @example
+ * ```tsx
+ * <RedirectAppLinks application={application}>


@cchaos Publicly-exposed constructs require comments like this for the auto-generated API docs.

https://github.com/elastic/kibana/blob/main/api_docs/shared_u_x.devdocs.json
https://github.com/elastic/kibana/blob/main/api_docs/shared_u_x.mdx

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.

clintandrewhall · 2022-03-09T20:26:53Z

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.tsx

+import { createNavigateToUrlClickHandler } from './click_handler';
+
+export interface RedirectAppLinksProps extends React.HTMLAttributes<HTMLDivElement> {
+  navigateToUrl: ApplicationStart['navigateToUrl'];


I'm not going to block the PR, but we'll need to refactor this to not rely on other plugins or services, even core. Feel free to add a TODO and assign it to me, as I'm working on this in the immediate term.

I opened #127464 to track this thanks

clintandrewhall · 2022-03-09T20:27:40Z

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.tsx

+import { ApplicationStart } from 'src/core/public';
+import { createNavigateToUrlClickHandler } from './click_handler';
+
+export interface RedirectAppLinksProps extends React.HTMLAttributes<HTMLDivElement> {


There's likely a better type for this, so data-test-subj and className would come along for free. I'll see what I can find.

Regardless, you could refactor this to be cleaner. Something like:

type Props = React.HTMLAttributes<HTMLDivElement> & Pick<ApplicationStart, 'navigateToUrl' | 'currentAppId$'; export interface RedirectAppLinksProps extends Props { className?: string; 'data-test-subj'?: string; }

EUI provides these (and aria-label) through CommonProps if that's helpful

Slightly cleaner now in dc63e36 thanks!

clintandrewhall · 2022-03-09T20:30:28Z

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.tsx

+  );
+
+  return (
+    // eslint-disable-next-line jsx-a11y/click-events-have-key-events


Need a comment why this is necessary. @cchaos do you have any concerns about this?

I always have concerns about adding onClick behavior to typically non-interactive components, but it looks like this is just to intercept any child hrefs. Other than that, I don't really know what this component is supposed to accomplish.

Hope this helps a bit: https://github.com/elastic/kibana/blob/main/src/plugins/kibana_react/public/page_template/no_data_page/no_data_card/elastic_agent_card.tsx#L74

src/plugins/shared_ux/public/components/redirect_app_links/utils.ts

clintandrewhall · 2022-03-09T20:32:56Z

src/plugins/shared_ux/public/components/redirect_app_links/utils.ts

+  container?: HTMLElement
+): HTMLAnchorElement | undefined => {
+  let current = element;
+  do {


Might not be entirely relevant, but it's something I think of whenever I see a do/while:

:-D It will probably make all of this code cleaner, too.

I prefer do...while over while(true) anytime.

…app_links

…irect_app_links.mdx Co-authored-by: Caroline Horn <[email protected]>

stacey-gammon

api doc snapshots look fine to me. The changes are unrelated to this PR, just looks like yarn jest was ran locally, which due to a recent change, will update the date on these. Something to look into fixing, otherwise these two files will always have changes every time yarn jest is run.

cchaos · 2022-03-10T22:00:16Z

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.stories.tsx

+        fill
+        iconType="plusInCircle"
+        onClick={action('button pressed')}
+      />


The button still needs a label/text it just didn't need to be internationalized 😄

Suggested change

/>

>Test link</EuiButton>

Thank you~ 2d2ddc1 for changes and for below comment as well :face_palm:

cchaos · 2022-03-10T22:01:52Z

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.tsx

+ *
+ * @example
+ * ```tsx
+ * <RedirectAppLinks application={application}>


These comments still need updating as the example still shows needing application/

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.stories.tsx

…irect_app_links.stories.tsx Co-authored-by: Caroline Horn <[email protected]>

kibana-ci · 2022-03-14T17:26:15Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`sharedUX`	49	55	+6

Async chunks

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

id	before	after	diff
`sharedUX`	100.7KB	102.1KB	+1.5KB

Page load bundle

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

id	before	after	diff
`sharedUX`	4.8KB	4.9KB	+49.0B

Unknown metric groups

async chunk count

id	before	after	diff
`sharedUX`	4	5	+1

ESLint disabled in files

id	before	after	diff
`sharedUX`	2	4	+2

ESLint disabled line counts

id	before	after	diff
`sharedUX`	1	2	+1

Total ESLint disabled count

id	before	after	diff
`sharedUX`	3	6	+3

History

💚 Build #29965 succeeded baf5e5a
💚 Build #29789 succeeded a16a9f9
💚 Build #29589 succeeded e2f1748
💛 Build #29444 was flaky 39fc016
💔 Build #29394 failed 8dbb2aa
💚 Build #28850 succeeded 40fbc24

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

initial commit

4b86cd3

rshen91 added loe:medium Medium Level of Effort backport:skip This commit does not require backporting Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Feb 1, 2022

rshen91 added 2 commits February 1, 2022 08:54

Merge remote-tracking branch 'upstream/main' into migrate-app_links

a7f0602

lint changes

db7f039

rshen91 mentioned this pull request Feb 1, 2022

[Meta] Transition Components to SharedUX From React Plugin #122961

Open

15 tasks

kibanamachine and others added 8 commits February 3, 2022 09:24

Merge branch 'main' into migrate-app_links

5bd07e5

wip storybook refactoring

ae9b70c

update test

4ea03c5

Merge remote-tracking branch 'upstream/main' into migrate-app_links

403dc58

fix namespace issue

d78d7e5

update unit test

c9c6fe4

Merge branch 'main' into migrate-app_links

73a2c83

Pass containerRef as a prop to Component

e8bdf23

rshen91 added 2 commits March 7, 2022 09:36

refactor

4560747

Merge branch 'main' into migrate-app_links

c1c39fe

rshen91 marked this pull request as ready for review March 8, 2022 14:39

rshen91 requested a review from a team March 8, 2022 14:39

rshen91 requested a review from a team as a code owner March 8, 2022 14:39

rshen91 added v8.2.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 8, 2022

spalger reviewed Mar 8, 2022

View reviewed changes

majagrubic reviewed Mar 8, 2022

View reviewed changes

rshen91 added 3 commits March 8, 2022 07:43

Merge remote-tracking branch 'upstream/main' into migrate-app_links

2c1700b

Merge remote-tracking branch 'origin/migrate-app_links' into migrate-…

fdd3b11

…app_links

consistent naming

9b2fcbe

cchaos reviewed Mar 8, 2022

View reviewed changes

clintandrewhall reviewed Mar 9, 2022

View reviewed changes

rshen91 added 2 commits March 10, 2022 07:07

Merge remote-tracking branch 'upstream/main' into migrate-app_links

2f9c870

Merge remote-tracking branch 'origin/migrate-app_links' into migrate-…

170f34c

…app_links

rshen91 mentioned this pull request Mar 10, 2022

[SharedUX] Refactor app links to avoid dependency on core #127464

Closed

rshen91 and others added 3 commits March 10, 2022 07:30

code review changes

dc63e36

Update src/plugins/shared_ux/public/components/redirect_app_links/red…

7bf4216

…irect_app_links.mdx Co-authored-by: Caroline Horn <[email protected]>

Merge branch 'main' into migrate-app_links

8dbb2aa

rshen91 requested review from majagrubic, cchaos and clintandrewhall March 10, 2022 14:49

Merge branch 'main' into migrate-app_links

39fc016

stacey-gammon approved these changes Mar 10, 2022

View reviewed changes

Merge branch 'main' into migrate-app_links

e2f1748

cchaos suggested changes Mar 10, 2022

View reviewed changes

rshen91 added 3 commits March 13, 2022 19:47

Merge remote-tracking branch 'upstream/main' into migrate-app_links

925e602

code review

2d2ddc1

merge origin

a16a9f9

rshen91 requested a review from cchaos March 14, 2022 01:56

majagrubic approved these changes Mar 14, 2022

View reviewed changes

Merge branch 'main' into migrate-app_links

baf5e5a

cchaos approved these changes Mar 14, 2022

View reviewed changes

src/plugins/shared_ux/public/components/redirect_app_links/redirect_app_links.stories.tsx Outdated Show resolved Hide resolved

rshen91 and others added 2 commits March 14, 2022 09:32

Update src/plugins/shared_ux/public/components/redirect_app_links/red…

c5183f3

…irect_app_links.stories.tsx Co-authored-by: Caroline Horn <[email protected]>

Merge branch 'main' into migrate-app_links

0cc5b2f

rshen91 enabled auto-merge (squash) March 14, 2022 15:33

Merge branch 'main' into migrate-app_links

efcebbc

rshen91 merged commit 9d8f95f into elastic:main Mar 14, 2022

rshen91 deleted the migrate-app_links branch March 14, 2022 17:51

maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022

[shared-ux] Migrate redirect app links component (elastic#124197)

2a4b003

rshen91 self-assigned this Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[shared-ux] Migrate redirect app links component #124197

[shared-ux] Migrate redirect app links component #124197

rshen91 commented Feb 1, 2022 •

edited

Loading

rshen91 commented Feb 3, 2022

majagrubic commented Mar 7, 2022

spalger left a comment

cchaos Mar 8, 2022

cchaos Mar 8, 2022

rshen91 Mar 8, 2022

clintandrewhall Mar 9, 2022

clintandrewhall Mar 9, 2022

rshen91 Mar 10, 2022

clintandrewhall Mar 9, 2022

cchaos Mar 9, 2022

rshen91 Mar 10, 2022

clintandrewhall Mar 9, 2022

cchaos Mar 9, 2022

majagrubic Mar 10, 2022

clintandrewhall Mar 9, 2022

majagrubic Mar 10, 2022

stacey-gammon left a comment

cchaos Mar 10, 2022

rshen91 Mar 14, 2022

cchaos Mar 10, 2022

kibana-ci commented Mar 14, 2022

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

[shared-ux] Migrate redirect app links component #124197

[shared-ux] Migrate redirect app links component #124197

Conversation

rshen91 commented Feb 1, 2022 • edited Loading

Summary

Checklist

For maintainers

rshen91 commented Feb 3, 2022

majagrubic commented Mar 7, 2022

spalger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Mar 14, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

rshen91 commented Feb 1, 2022 •

edited

Loading