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

Initial version of relevance tools with Search Comparison UI #5

Merged
merged 6 commits into from
Oct 28, 2022

Conversation

mingkunm
Copy link
Contributor

@mingkunm mingkunm commented Oct 25, 2022

Signed-off-by: Mingkun Ma mingkunm@amazon.com

Description

Add basic mocks, types, service, etc.

Note

This is a kick-off PR, and doesn't include the most up-to-date Results Compare page.

Issues Resolved

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mingkun Ma <mingkunm@amazon.com>
translations/ja-JP.json Outdated Show resolved Hide resolved
Comment on lines +11 to +27
export const CreateIndex = () => {
return (
<>
<Header />
<EuiPageBody>
<EuiEmptyPrompt
title={<h2>Create an index to start comparing search results. </h2>}
body={
<p>
Before you can query data, you have to index it.{' '}
<EuiLink>Learn how to index your data.</EuiLink>
</p>
}
/>
</EuiPageBody>
</>
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this going to be part of the final plugin? Or was this copied over from somewhere?

In practice, I don't think anyone would want to use the query comparison plugin to create a new index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This page is from the mock. User will get into this page if there is no index exists.

Copy link
Collaborator

@macohen macohen Oct 27, 2022

Choose a reason for hiding this comment

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

Agreed about this in practice, @msfroh, but someone may find their way to the comparison plugin without indexing anything in some odd way so the UI should account for that. Can we resolve this as is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we resolve this as is?

The only change I might make would be renaming the file to something like no_available_indexes.tsx. The purpose of the page isn't to create an index, but rather to serve a useful error page if there are no indexes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only change I might make would be renaming the file to something like no_available_indexes.tsx.

I agree with the new file name.

Speaking with available indexes, should we let users select all indexes returned from this call? I saw individual index has health and status attributes. Do we want to display all of them in Select Index Dropdown?

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw individual index has health and status attributes. Do we want to display all of them in Select Index Dropdown?

I think for now, we can keep it simple and show all indices returned from /_cat/indices. If an index is unable to handle a search request, we should surface an error. An index is "red" state is not usual, so IMO we don't need to worry about it too much for now.

In the long run, maybe we'll hide the indexes that start with . by default? Again, not something I would worry about for now.

public/types/index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
common/index.ts Outdated Show resolved Hide resolved
public/constants/index.ts Outdated Show resolved Hide resolved
public/components/query_compare/home.tsx Show resolved Hide resolved
public/components/query_compare/search_result/index.tsx Outdated Show resolved Hide resolved
public/components/query_compare/search_result/index.tsx Outdated Show resolved Hide resolved
public/components/query_compare/search_result/index.tsx Outdated Show resolved Hide resolved
public/components/query_compare/search_result/index.tsx Outdated Show resolved Hide resolved
Signed-off-by: Mingkun Ma <mingkunm@amazon.com>
@mingkunm
Copy link
Contributor Author

@ps48 @msfroh New commit resolved comments. Thanks!

public/constants/index.ts Outdated Show resolved Hide resolved
Signed-off-by: Mingkun Ma <mingkunm@amazon.com>
opensearch_dashboards.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@macohen macohen added the v3.0.0 label Oct 27, 2022
Signed-off-by: Mingkun Ma <mingkunm@amazon.com>
@macohen macohen mentioned this pull request Oct 27, 2022
23 tasks
package.json Outdated Show resolved Hide resolved
Signed-off-by: Mingkun Ma <mingkunm@amazon.com>
Comment on lines +46 to +47
const [querqyResult1, setQuerqyResult1] = useState({});
const [querqyResult2, setQuerqyResult2] = useState({});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we find/replace all occurrences of querqyResult with queryResult?

Given that there is a thing called Querqy, this apparent typo is confusing.

Signed-off-by: Mingkun Ma <mingkunm@amazon.com>
@macohen macohen merged commit 862a03b into opensearch-project:main Oct 28, 2022
@macohen macohen changed the title Add basic mocks Initial version of relevance tools with Search Comparison UI Oct 31, 2022
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.

4 participants