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

[Discover] Add Footer Bar for Single Line Editor #8565

Merged
merged 9 commits into from
Oct 23, 2024

Conversation

sejli
Copy link
Member

@sejli sejli commented Oct 11, 2024

Description

  • Adds footer bar in multiline editor to single line editor when editor is focused
  • Fixes query result component not displaying errors
  • Correctly surfaces errors from async search strategy

Issues Resolved

Screenshot

image image image image image

Testing the changes

Changelog

  • feat: Adds editor footer to single line editor on focus

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.

Copy link

codecov bot commented Oct 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 60.86%. Comparing base (79fed30) to head (c8278f3).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ins/data/public/ui/query_editor/editors/shared.tsx 0.00% 21 Missing ⚠️
...ic/application/view_components/utils/use_search.ts 0.00% 5 Missing ⚠️
...query_string/language_service/lib/query_result.tsx 0.00% 2 Missing ⚠️
src/plugins/data/common/utils/helpers.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8565      +/-   ##
==========================================
- Coverage   60.86%   60.86%   -0.01%     
==========================================
  Files        3793     3793              
  Lines       90447    90480      +33     
  Branches    14203    14209       +6     
==========================================
+ Hits        55053    55069      +16     
- Misses      31906    31922      +16     
- Partials     3488     3489       +1     
Flag Coverage Δ
Linux_1 29.07% <0.00%> (-0.02%) ⬇️
Linux_2 56.40% <0.00%> (ø)
Linux_3 ?
Linux_4 29.84% <0.00%> (-0.02%) ⬇️
Windows_1 29.10% <0.00%> (+<0.01%) ⬆️
Windows_2 56.35% <0.00%> (ø)
Windows_3 37.67% <0.00%> (-0.01%) ⬇️
Windows_4 29.84% <0.00%> (-0.02%) ⬇️

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.

@@ -215,3 +215,40 @@
display: block;
}
}

.queryEditor__footer {
Copy link
Member

Choose a reason for hiding this comment

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

@sejli lets fix this in a fast follow


.euiFormControlLayout--group .osdQuerEditor__singleLine .euiText {
background-color: unset !important;
line-height: 21px !important;
Copy link
Member

Choose a reason for hiding this comment

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

Discover as a whole has a lot of overrides. This was an accepted deviation given the timelines. we will need to discuss how much of this needs to move to OUI vs stay custom long term

@@ -22,10 +22,9 @@ export interface QueryStatus {
status: ResultStatus;
body?: {
error?: {
reason?: string;
details: string;
statusCode?: number;
Copy link
Member

Choose a reason for hiding this comment

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

why do we have this here? We dont want to show the user the status code, but rather the error and the reason

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

The PR is good, has some follow up work though. Lets follow up on how we display the error. I think for now this is better than what we have, but we need to fix:

  1. Error when the editor is not focussed
  2. How the error is displayed
  3. Loading state when the query is run after the initial load

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. There are some open questions, but none are blocking

return res.custom({
statusCode: err.name,
statusCode: error.status,
Copy link
Collaborator

Choose a reason for hiding this comment

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

all error types above won't necessarily have status, right? Is that okay?

@virajsanghvi virajsanghvi merged commit c11a801 into opensearch-project:main Oct 23, 2024
66 of 69 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-8565-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c11a801410fff1ea19f1debeb517cd9c5018b7c6
# Push it to GitHub
git push --set-upstream origin backport/backport-8565-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-8565-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-8565-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c11a801410fff1ea19f1debeb517cd9c5018b7c6
# Push it to GitHub
git push --set-upstream origin backport/backport-8565-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-8565-to-2.x.

ananzh pushed a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Oct 23, 2024
…8565)

* initial commit for single line editor footer

Signed-off-by: Sean Li <[email protected]>

* fixing styling and functionality

Signed-off-by: Sean Li <[email protected]>

* Changeset file for PR opensearch-project#8565 created/updated

* fixing bug with error not showing up in footer

Signed-off-by: Sean Li <[email protected]>

* fixing loading state thanks ashwinpc

Signed-off-by: Sean Li <[email protected]>

* trying to surface errors

Signed-off-by: Sean Li <[email protected]>

* adding new error for error state

Signed-off-by: Sean Li <[email protected]>

* Revert "fixing loading state thanks ashwinpc"

This reverts commit 64b5969.

* correctly passing async search strat errors

Signed-off-by: Sean Li <[email protected]>

---------

Signed-off-by: Sean Li <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
ananzh pushed a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Oct 23, 2024
…opensearch-project#8565)

Backport PR:
opensearch-project#8565

From the original PR:

* initial commit for single line editor footer

Signed-off-by: Sean Li <[email protected]>

* fixing styling and functionality

Signed-off-by: Sean Li <[email protected]>

* Changeset file for PR opensearch-project#8565 created/updated

* fixing bug with error not showing up in footer

Signed-off-by: Sean Li <[email protected]>

* fixing loading state thanks ashwinpc

Signed-off-by: Sean Li <[email protected]>

* trying to surface errors

Signed-off-by: Sean Li <[email protected]>

* adding new error for error state

Signed-off-by: Sean Li <[email protected]>

* Revert "fixing loading state thanks ashwinpc"

This reverts commit 64b5969.

* correctly passing async search strat errors

Signed-off-by: Sean Li <[email protected]>

---------

Signed-off-by: Sean Li <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
ananzh added a commit that referenced this pull request Oct 23, 2024
…#8565) (#8691)

* [2.x-manual-backport][Discover] Add Footer Bar for Single Line Editor (#8565)

Backport PR:
#8565

From the original PR:

* initial commit for single line editor footer

Signed-off-by: Sean Li <[email protected]>

* fixing styling and functionality

Signed-off-by: Sean Li <[email protected]>

* Changeset file for PR #8565 created/updated

* fixing bug with error not showing up in footer

Signed-off-by: Sean Li <[email protected]>

* fixing loading state thanks ashwinpc

Signed-off-by: Sean Li <[email protected]>

* trying to surface errors

Signed-off-by: Sean Li <[email protected]>

* adding new error for error state

Signed-off-by: Sean Li <[email protected]>

* Revert "fixing loading state thanks ashwinpc"

This reverts commit 64b5969.

* correctly passing async search strat errors

Signed-off-by: Sean Li <[email protected]>

---------

Signed-off-by: Sean Li <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

* fix snapshot

Signed-off-by: Anan Zhuang <[email protected]>

---------

Signed-off-by: Sean Li <[email protected]>
Signed-off-by: Anan Zhuang <[email protected]>
Co-authored-by: Sean Li <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Qxisylolo pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 30, 2024
…8565)

* initial commit for single line editor footer

Signed-off-by: Sean Li <[email protected]>

* fixing styling and functionality

Signed-off-by: Sean Li <[email protected]>

* Changeset file for PR opensearch-project#8565 created/updated

* fixing bug with error not showing up in footer

Signed-off-by: Sean Li <[email protected]>

* fixing loading state thanks ashwinpc

Signed-off-by: Sean Li <[email protected]>

* trying to surface errors

Signed-off-by: Sean Li <[email protected]>

* adding new error for error state

Signed-off-by: Sean Li <[email protected]>

* Revert "fixing loading state thanks ashwinpc"

This reverts commit 64b5969.

* correctly passing async search strat errors

Signed-off-by: Sean Li <[email protected]>

---------

Signed-off-by: Sean Li <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
@@ -85,8 +85,14 @@ export function defineSearchStrategyRouteProvider(logger: Logger, router: IRoute
const queryRes: IDataFrameResponse = await searchStrategy.search(context, req as any, {});
return res.ok({ body: { ...queryRes } });
} catch (err) {
let error;
try {
error = JSON.parse(err.message);
Copy link
Member

@joshuali925 joshuali925 Oct 31, 2024

Choose a reason for hiding this comment

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

the status code might be in err object, not in err.message. If err.message is a valid json string, then status code is lost

return res.custom({
statusCode: err.name,
statusCode: error.status,
Copy link
Member

Choose a reason for hiding this comment

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

where is error.status coming from?

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.

7 participants