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

Highlight the anchor row in surrounding doc view #7025

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

abbyhu2000
Copy link
Member

Description

Highlight the anchor row in surrounding doc view.

Issues Resolved

resolves #6457

Screenshot

Screenshot 2024-06-13 at 3 04 49 PM

Changelog

  • fix:Highlight the anchor row in surrounding doc view

Signed-off-by: abbyhu2000 <[email protected]>
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.44%. Comparing base (190dab0) to head (fa81e81).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7025   +/-   ##
=======================================
  Coverage   67.44%   67.44%           
=======================================
  Files        3444     3444           
  Lines       67873    67873           
  Branches    11029    11029           
=======================================
  Hits        45777    45777           
  Misses      19431    19431           
  Partials     2665     2665           
Flag Coverage Δ
Linux_1 33.07% <ø> (ø)
Linux_2 55.11% <ø> (ø)
Linux_3 45.22% <ø> (ø)
Linux_4 34.86% <ø> (ø)
Windows_1 33.10% <ø> (ø)
Windows_2 55.06% <ø> (ø)
Windows_3 45.24% <ø> (ø)
Windows_4 34.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -48,7 +48,7 @@ const TableRowUI = ({
]);

const tableRow = (
<tr key={row._id}>
<tr key={row._id} className={row.isAnchor ? 'osdDocTable__row--highlight' : ''}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a test for this or is it covered in some other way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think default discover table is covered by any unit tests. src/plugins/discover/public/application/components/default_discover_table

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a ftr test? cuz we want to gradually build up the test coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we want to add unit tests and functional tests for default discover table overall. Created an issue to track it here: #7048

@@ -32,7 +32,9 @@ import { ComponentType } from 'react';
import { SearchResponse } from 'elasticsearch';
import { IndexPattern } from '../../../../data/public';

export type OpenSearchSearchHit<T = unknown> = SearchResponse<T>['hits']['hits'][number];
export type OpenSearchSearchHit<T = unknown> = SearchResponse<T>['hits']['hits'][number] & {
Copy link
Member

Choose a reason for hiding this comment

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

nit: the name OpenSearchSearchHit is confusing for me because it seems like OpenSearch is returning back to us if it is an anchor or not. Can we create another alias for this to be more descriptive that shims to the hit response if it isAnchor or not based on the context?

that would help me as well not worry if there is potential issues changing the type (even though it is just an optional field). if this type is actually only used within Discover in context then perhaps the name can be reconsidered since OpenSearchSearchHit implies to me it has more purposes than just for table rows.

Copy link
Collaborator

@virajsanghvi virajsanghvi left a comment

Choose a reason for hiding this comment

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

approving assuming its going to be tested in tracked issue

@abbyhu2000 abbyhu2000 requested a review from kavilla June 19, 2024 19:12
@abbyhu2000 abbyhu2000 merged commit 4b78b36 into opensearch-project:main Jun 20, 2024
69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 20, 2024
(cherry picked from commit 4b78b36)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mengweieric pushed a commit to mengweieric/OpenSearch-Dashboards that referenced this pull request Jun 24, 2024
LDrago27 pushed a commit that referenced this pull request Jul 25, 2024
(cherry picked from commit 4b78b36)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] When using the "View surrounding documents" function the main event is not highlighted
4 participants