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

Add query result and time to the query editor footer #7951

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

abbyhu2000
Copy link
Member

@abbyhu2000 abbyhu2000 commented Aug 30, 2024

Description

Add query result and query execution time to the query editor footer.

TODO:

  • query error UI in the follow up PR

Issues Resolved

Screenshot

Screen.Recording.2024-09-03.at.11.36.07.AM.mov

Testing the changes

Changelog

  • feat: Add query result and time to the query editor footer

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

@abbyhu2000 abbyhu2000 changed the title Query error try 2 Add query result and time to the query editor footer Sep 3, 2024
@abbyhu2000 abbyhu2000 marked this pull request as ready for review September 3, 2024 18:32
Copy link
Contributor

github-actions bot commented Sep 3, 2024

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

opensearch-changeset-bot bot added a commit to abbyhu2000/OpenSearch-Dashboards that referenced this pull request Sep 3, 2024
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

generally looks good. just clean up

src/plugins/data/common/data_frames/types.ts Outdated Show resolved Hide resolved
anchorPosition={'downRight'}
>
<EuiPopoverTitle>ERRORS</EuiPopoverTitle>
<div style={{ width: '250px' }}>
Copy link
Member

Choose a reason for hiding this comment

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

add a stylesheet

@@ -34,7 +34,7 @@ export interface TopNavProps {

export const TopNav = ({ opts, showSaveQuery, isEnhancementsEnabled }: TopNavProps) => {
const { services } = useOpenSearchDashboards<DiscoverViewServices>();
const { inspectorAdapters, savedSearch, indexPattern } = useDiscoverContext();
const { data$, inspectorAdapters, savedSearch, indexPattern } = useDiscoverContext();
Copy link
Member

Choose a reason for hiding this comment

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

if we only require do we think we should maybe just create a new observable that this listens to instead of subscribing to a lot of data in the top nav?

@@ -50,6 +51,10 @@ export interface SearchData {
bucketInterval?: TimechartHeaderBucketInterval | {};
chartData?: Chart;
title?: string;
statusBody?: {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

i still think maybe we maybe we can:

status: ResultStatus | DetailedResultStatus

and then the DetailedResultStatus is

status: ResultStatus,
message: string,
statusCode?: number,
elapsedMs?: number,


if (queryStatus.status === 'ready') {
return (
<EuiButtonEmpty iconSide="left" iconType={'checkInCircleEmpty'} size="xs" onClick={() => {}}>
Copy link
Member

Choose a reason for hiding this comment

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

is it expected that clicking on this button doesn't do anything?

yarn.lock Outdated Show resolved Hide resolved
src/plugins/data/public/ui/search_bar/search_bar.tsx Outdated Show resolved Hide resolved
@abbyhu2000 abbyhu2000 force-pushed the query_error_try_2 branch 2 times, most recently from f1a273a to 8051384 Compare September 3, 2024 23:31
@ashwin-pc ashwin-pc added the discover for discover reinvent label Sep 4, 2024
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
@@ -24,6 +27,7 @@ export interface UiServiceStartDependencies {

export class UiService implements Plugin<IUiSetup, IUiStart> {
enhancementsConfig: ConfigSchema['enhancements'];
private queryStatus$ = new BehaviorSubject<QueryStatus>({ status: ResultStatus.READY });
Copy link
Member

Choose a reason for hiding this comment

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

We should not do this. This would mean that we are duplicating state information in multiple places and no longer have a single source of truth. If we forget to set or clear the status correctly, the UI could show both an error and the correct result at the same time, which could lead to bugs. If we are afraid of prop drilling, lets use react context within the TopNav of the data plugin, but not have 2 places that have the same state.

Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this now?

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
@@ -42,16 +42,9 @@ export const removeKeyword = (queryString: string | undefined) => {
};

export const handleFacetError = (response: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

We should add a test for this

Copy link
Member

Choose a reason for hiding this comment

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

A lot of components depend on this hook working as expected, we should add some tests here

@@ -24,6 +27,7 @@ export interface UiServiceStartDependencies {

export class UiService implements Plugin<IUiSetup, IUiStart> {
enhancementsConfig: ConfigSchema['enhancements'];
private queryStatus$ = new BehaviorSubject<QueryStatus>({ status: ResultStatus.READY });
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this now?

src/plugins/data/public/ui/types.ts Outdated Show resolved Hide resolved
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
@kavilla kavilla merged commit 7e8612f into opensearch-project:main Sep 4, 2024
67 of 69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 4, 2024
* add query result to the footer

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* fix unit test

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* conditional language

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* native language still use toast

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* refactor to not use two states

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* address comments

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

---------

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
(cherry picked from commit 7e8612f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 4, 2024
* add query result to the footer

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* fix unit test

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* conditional language

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* native language still use toast

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* refactor to not use two states

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* address comments

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

---------

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
(cherry picked from commit 7e8612f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ashwin-pc pushed a commit that referenced this pull request Sep 5, 2024
* add query result to the footer



* fix unit test



* conditional language



* native language still use toast



* refactor to not use two states



* address comments



---------


(cherry picked from commit 7e8612f)

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
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>
kavilla pushed a commit that referenced this pull request Sep 5, 2024
* add query result to the footer



* fix unit test



* conditional language



* native language still use toast



* refactor to not use two states



* address comments



---------


(cherry picked from commit 7e8612f)

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
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>
Hailong-am pushed a commit to Hailong-am/OpenSearch-Dashboards that referenced this pull request Sep 5, 2024
…ect#7951)

* add query result to the footer

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* fix unit test

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* conditional language

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* native language still use toast

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* refactor to not use two states

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* address comments

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

---------

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.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.

5 participants