Add DocView links pluggable injection capability #1200

RoyiSitbon · 2022-02-02T09:54:16Z

Signed-off-by: sitbubu [email protected]

Description

Currently, we don't have an option to assimilate links under our top doc view section.
The “view surrounding documents” and “view single document” were written as static content.
We would like to make the top right side under doc view to be pluggable,
in order to assimilate new abilities such as “open in new tab icons” etc.

Before:

After:

Additional info under linked issue:
#1199

test/plugin_functional/plugins/doc_views_links_plugin/public/plugin.tsx

ashwin-pc · 2022-02-22T01:58:11Z

Nice PR. I just had one question. In your Before and After images, there is an "open in new tab" icon (EUI popout icon). The EuiListGroup element can show icons but your PR doesn't seem to use that. How have you achieved that?

RoyiSitbon · 2022-02-22T08:27:26Z

Nice PR. I just had one question. In your Before and After images, there is an "open in new tab" icon (EUI popout icon). The EuiListGroup element can show icons but your PR doesn't seem to use that. How have you achieved that?

Hey, thanks for replying. It was an example, basically, you can set any icon you want. e.g:

ashwin-pc · 2022-02-22T08:35:16Z

Hey, thanks for replying. It was an example, basically, you can set any icon you want.

Oh right, I missed the how you were extending EuiListGroupItemProps. This looks good to me 👍 . It looks like your PR is breaking some functional tests in discover though.

RoyiSitbon · 2022-02-22T09:55:25Z

Hey, thanks for replying. It was an example, basically, you can set any icon you want.

Oh right, I missed the how you were extending EuiListGroupItemProps. This looks good to me 👍 . It looks like your PR is breaking some functional tests in discover though.

Thanks, added a small fix.
Hopefully, all will pass as expected now.

ashwin-pc · 2022-02-22T10:37:07Z

src/plugins/discover/public/application/components/doc_viewer_links/doc_viewer_links.tsx

+      if (item.generateurlcb) {
+        item.href = item.generateurlcb(renderProps);
+        delete item.generateurlcb;
+      }


Sorry i didnt catch this earlier but we should avoid mutating the docViewLink object directly. We can return a new object instead e.g.

.map((item) => { const { generateurlcb, href, ...props } = item; const listItem: EuiListGroupItemProps = { ...props, href: generateurlcb ? generateurlcb(renderProps) : href, } return listItem; }

This way we also ensure that the new object has only the props that the EuiListGroup component accepts.

amazing, thanks

ashwin-pc · 2022-02-22T10:48:23Z

src/plugins/discover/public/application/doc_views_links/doc_views_links_types.ts

@@ -10,7 +10,7 @@ import { IndexPattern } from '../../../../data/public';
 export interface DocViewLink extends EuiListGroupItemProps {
  href?: string;
  order: number;
-  generateurlfn?(renderProps: any): string;
+  generateurlcb?(renderProps: any): string;


Any reason this property isn't camel cased?

Not really, changed.

RoyiSitbon · 2022-03-01T09:01:04Z

@ashwin-pc , it passed all the tests. Thanks

ashwin-pc

Thanks for making the changes. This looks good to me

kavilla · 2022-03-03T19:21:41Z

@RoyiSitbon, can you run yarn test:jest -u and push the snapshot update?

RoyiSitbon · 2022-03-03T20:50:01Z

@RoyiSitbon, can you run yarn test:jest -u and push the snapshot update?

done :)

ashwin-pc

Thanks for that, LGTM

kavilla · 2022-03-04T21:00:26Z

src/plugins/discover/public/application/components/doc_viewer_links/doc_viewer_links.scss

@@ -0,0 +1,21 @@
+.osdDocViewerLinks {


I guess we don't lint scss files? I wonder if we broke something with the linter or perhaps we need to add it to the linter file.

In the meantime, I see most of our scss files are two space indented and I think some of the indention could use some loving.

kavilla · 2022-03-04T21:02:53Z

src/plugins/discover/public/application/angular/doc_table/components/table_row/details.html

-            data-test-subj="docTableRowAction"
-            ng-href="{{ getContextAppHref() }}"
-            ng-if="indexPattern.isTimeBased()"
-            i18n-id="discover.docTable.tableRow.viewSurroundingDocumentsLinkText"


Is there a way to utilize i18n within the updated component once we have i18n translations?

sure, added.

kavilla · 2022-03-04T21:04:17Z

src/plugins/discover/public/application/angular/doc_table/components/table_row/details.html

-            class="euiLink"
-            data-test-subj="docTableRowAction"
-            ng-href="{{ getContextAppHref() }}"
-            ng-if="indexPattern.isTimeBased()"


I'm not seeing this logic in the src/plugins/discover/public/plugin.ts, could we include that?

kavilla · 2022-03-04T21:45:29Z

I think this is an awesome change! I have some comments, I'm also wondering where can include some of this information of how to access this functionality as a plugin as it's not really called out anywhere. Perhaps we can tack on this information on this issue: opensearch-project/documentation-website#171?

kavilla · 2022-03-07T19:32:58Z

@RoyiSitbon, apologies we were targeting to get this into the release for 1.3.0 but as pens down was last week Friday [issue]. We will target this for the next release.

Again, sorry about the inconvenience but thank you for the awesome changes!

Signed-off-by: sitbubu <[email protected]>

kavilla

PR was updated with main.

Re-approving. This looks good to me @tmarkley, @ashwin-pc, have any objections? I think we can target for 2.2 as well.

ananzh

LGTM 💚
Thanks for converting part of the detail view to react component, registering it and fixing the link

RoyiSitbon · 2022-07-11T07:00:54Z

💚

💚

* Add DocView links pluggable injection capability * Add proper e2e + bugfix * set prop as lower case * Better name * Delete prop after usage * Camel case + avoid mutating * Added right data-test-subj in order to fix tests * update snapshot * Add hide prop in order to mimic ng-if condition * Add i18n translation support * Fix tests * Made text smaller + used eui components instead of custom css * Snapshots Signed-off-by: sitbubu <[email protected]> (cherry picked from commit c33bf8c)

* Add DocView links pluggable injection capability * Add proper e2e + bugfix * set prop as lower case * Better name * Delete prop after usage * Camel case + avoid mutating * Added right data-test-subj in order to fix tests * update snapshot * Add hide prop in order to mimic ng-if condition * Add i18n translation support * Fix tests * Made text smaller + used eui components instead of custom css * Snapshots Signed-off-by: sitbubu <[email protected]> Signed-off-by: Royi Sitbon <[email protected]> (cherry picked from commit c33bf8c)

…1200) * Add DocView links pluggable injection capability * Add proper e2e + bugfix * set prop as lower case * Better name * Delete prop after usage * Camel case + avoid mutating * Added right data-test-subj in order to fix tests * update snapshot * Add hide prop in order to mimic ng-if condition * Add i18n translation support * Fix tests * Made text smaller + used eui components instead of custom css * Snapshots Signed-off-by: sitbubu <[email protected]>

* Add DocView links pluggable injection capability * Add proper e2e + bugfix * set prop as lower case * Better name * Delete prop after usage * Camel case + avoid mutating * Added right data-test-subj in order to fix tests * update snapshot * Add hide prop in order to mimic ng-if condition * Add i18n translation support * Fix tests * Made text smaller + used eui components instead of custom css * Snapshots Signed-off-by: sitbubu <[email protected]> Signed-off-by: Royi Sitbon <[email protected]> (cherry picked from commit c33bf8c) Co-authored-by: Royi Sitbon <[email protected]>

…1200) * Add DocView links pluggable injection capability * Add proper e2e + bugfix * set prop as lower case * Better name * Delete prop after usage * Camel case + avoid mutating * Added right data-test-subj in order to fix tests * update snapshot * Add hide prop in order to mimic ng-if condition * Add i18n translation support * Fix tests * Made text smaller + used eui components instead of custom css * Snapshots Signed-off-by: sitbubu <[email protected]>

RoyiSitbon requested a review from a team as a code owner February 2, 2022 09:54

kavilla linked an issue Feb 3, 2022 that may be closed by this pull request

Top Right docViewer linkable plugin injection #1199

Closed

kavilla reviewed Feb 9, 2022

View reviewed changes

test/plugin_functional/plugins/doc_views_links_plugin/public/plugin.tsx Outdated Show resolved Hide resolved

RoyiSitbon force-pushed the AddDocViewPlugableLinkInjection branch from 57d308f to 944f7c3 Compare February 16, 2022 08:01

RoyiSitbon requested a review from kavilla February 18, 2022 14:00

RoyiSitbon force-pushed the AddDocViewPlugableLinkInjection branch 2 times, most recently from 56bd0e7 to 9f0c57b Compare February 19, 2022 15:43

kavilla assigned ashwin-pc Feb 21, 2022

ashwin-pc reviewed Feb 22, 2022

View reviewed changes

RoyiSitbon force-pushed the AddDocViewPlugableLinkInjection branch from 6b62b70 to 7a25121 Compare February 22, 2022 12:24

RoyiSitbon requested a review from ashwin-pc February 22, 2022 13:25

RoyiSitbon force-pushed the AddDocViewPlugableLinkInjection branch from 71712a3 to b77de09 Compare March 1, 2022 06:49

ashwin-pc previously approved these changes Mar 2, 2022

View reviewed changes

RoyiSitbon dismissed ashwin-pc’s stale review via a4a5a56 March 3, 2022 20:49

RoyiSitbon requested a review from ashwin-pc March 3, 2022 20:50

kavilla added the v1.3.0 label Mar 4, 2022

ashwin-pc previously approved these changes Mar 4, 2022

View reviewed changes

kavilla reviewed Mar 4, 2022

View reviewed changes

kavilla removed the v1.3.0 label Mar 7, 2022

RoyiSitbon added 13 commits June 22, 2022 12:06

Add DocView links pluggable injection capability

cea96a3

Signed-off-by: sitbubu <[email protected]>

Add proper e2e + bugfix

0eeac1d

Signed-off-by: sitbubu <[email protected]>

set prop as lower case

9410612

Signed-off-by: sitbubu <[email protected]>

Better name

09ba5ce

Signed-off-by: sitbubu <[email protected]>

Delete prop after usage

7ffec20

Signed-off-by: sitbubu <[email protected]>

Camel case + avoid mutating

353c2db

Signed-off-by: sitbubu <[email protected]>

Added right data-test-subj in order to fix tests

7dde376

Signed-off-by: sitbubu <[email protected]>

update snapshot

3298b42

Signed-off-by: sitbubu <[email protected]>

Add hide prop in order to mimic ng-if condition

983241c

Signed-off-by: sitbubu <[email protected]>

Add i18n translation support

76d6dd4

Signed-off-by: sitbubu <[email protected]>

Fix tests

38934ef

Signed-off-by: sitbubu <[email protected]>

Made text smaller + used eui components instead of custom css

ab2b1b3

Signed-off-by: sitbubu <[email protected]>

Snapshots

5ec302b

Signed-off-by: sitbubu <[email protected]>

joshuarrrr force-pushed the AddDocViewPlugableLinkInjection branch from 5e7e53c to 5ec302b Compare June 22, 2022 19:06

manasvinibs dismissed kavilla’s stale review via 5ec302b June 22, 2022 19:58

kavilla approved these changes Jun 23, 2022

View reviewed changes

joshuarrrr added the v2.2.0 label Jun 23, 2022

kavilla added the backport 2.x label Jul 4, 2022

joshuarrrr added the needs-documentation label Jul 7, 2022

opensearch-trigger-bot bot mentioned this pull request Aug 16, 2022

Add DocView links pluggable injection capability #2147

Open

ananzh approved these changes Jul 7, 2022

View reviewed changes

kavilla merged commit c33bf8c into opensearch-project:main Jul 13, 2022

opensearch-trigger-bot bot mentioned this pull request Jul 13, 2022

[Backport 2.x] Add DocView links pluggable injection capability #1891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DocView links pluggable injection capability #1200

Add DocView links pluggable injection capability #1200

RoyiSitbon commented Feb 2, 2022

ashwin-pc commented Feb 22, 2022

RoyiSitbon commented Feb 22, 2022

ashwin-pc commented Feb 22, 2022

RoyiSitbon commented Feb 22, 2022

ashwin-pc Feb 22, 2022

RoyiSitbon Feb 22, 2022

ashwin-pc Feb 22, 2022

RoyiSitbon Feb 22, 2022

RoyiSitbon commented Mar 1, 2022

ashwin-pc left a comment

kavilla commented Mar 3, 2022

RoyiSitbon commented Mar 3, 2022

ashwin-pc left a comment

kavilla Mar 4, 2022 •

edited

Loading

RoyiSitbon Mar 8, 2022

kavilla Mar 4, 2022

RoyiSitbon Mar 8, 2022

kavilla Mar 4, 2022

RoyiSitbon Mar 8, 2022

kavilla commented Mar 4, 2022

kavilla commented Mar 7, 2022

kavilla left a comment

ananzh left a comment

RoyiSitbon commented Jul 11, 2022

Add DocView links pluggable injection capability #1200

Add DocView links pluggable injection capability #1200

Conversation

RoyiSitbon commented Feb 2, 2022

Description

ashwin-pc commented Feb 22, 2022

RoyiSitbon commented Feb 22, 2022

ashwin-pc commented Feb 22, 2022

RoyiSitbon commented Feb 22, 2022

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

RoyiSitbon commented Mar 1, 2022

ashwin-pc left a comment

Choose a reason for hiding this comment

kavilla commented Mar 3, 2022

RoyiSitbon commented Mar 3, 2022

ashwin-pc left a comment

Choose a reason for hiding this comment

kavilla Mar 4, 2022 • edited Loading

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

kavilla commented Mar 4, 2022

kavilla commented Mar 7, 2022

kavilla left a comment

Choose a reason for hiding this comment

ananzh left a comment

Choose a reason for hiding this comment

RoyiSitbon commented Jul 11, 2022

kavilla Mar 4, 2022 •

edited

Loading