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

[MD] Address UX comments for index pattern pages #2505

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

kristenTian
Copy link
Contributor

@kristenTian kristenTian commented Oct 5, 2022

Signed-off-by: Kristen Tian [email protected]

Description

Address UX comments for index pattern pages

  1. Switch from SaveObjectFinder to EUISelectable

image

  1. Add data source title to step index pattern

image

  1. Error toast while fetching existing data sources

image

Issues Resolved

UX
opensearch-project/ux#42

Check List

  • Commits are signed per the DCO using --signoff

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2022

Codecov Report

Merging #2505 (8151000) into main (ca718a3) will decrease coverage by 0.02%.
The diff coverage is 38.63%.

@@            Coverage Diff             @@
##             main    #2505      +/-   ##
==========================================
- Coverage   66.78%   66.76%   -0.03%     
==========================================
  Files        3201     3201              
  Lines       60913    60943      +30     
  Branches     9254     9258       +4     
==========================================
+ Hits        40683    40689       +6     
- Misses      18018    18039      +21     
- Partials     2212     2215       +3     
Impacted Files Coverage Δ
...ndex_pattern_management/public/components/utils.ts 40.90% <0.00%> (-23.38%) ⬇️
...ents/step_data_source/components/header/header.tsx 61.11% <41.66%> (-28.89%) ⬇️
...d/components/step_data_source/step_data_source.tsx 66.66% <50.00%> (ø)
...ts/step_index_pattern/components/header/header.tsx 63.63% <55.55%> (+3.63%) ⬆️
...mponents/step_index_pattern/step_index_pattern.tsx 85.71% <100.00%> (ø)
...ic/application/models/sense_editor/sense_editor.ts 64.00% <0.00%> (-0.89%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kristenTian kristenTian force-pushed the OSD-main branch 2 times, most recently from 9b07fb6 to 0caf256 Compare October 5, 2022 03:24
@KrooshalUX
Copy link

@kristenTian Can you please provide a full screen screenshot? I am not understanding the context of the radio button. Please also see : opensearch-project/ux#40

zhongnansu
zhongnansu previously approved these changes Oct 5, 2022
@kristenTian
Copy link
Contributor Author

@kristenTian Can you please provide a full screen screenshot? I am not understanding the context of the radio button. Please also see : opensearch-project/ux#40

Updated

Comment on lines +63 to +65
if (fetchedDataSources?.length) {
setDataSources(fetchedDataSources);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it guarantied that the backend will throw if no data sources are available? If not, shouldn't we show something when no data sources are returned?

Copy link
Contributor 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 backend will throw. Current behavior is the drop down will show as empty. in later phases, UX designed an empty state to show.

.find<DataSourceAttributes>({
type: 'data-source',
fields: ['title', 'type'],
perPage: 10000,
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 any precedence for this 10000? Does it need to exist? Does it have to be hardcoded? Can it be configurable? Can it be read from a common config?

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is referenced from the how other places are using this method in the repo:

and

I am also not a fan of this approach. We can add an action item to add pagination supported client wrapper as an action item?

Copy link
Member

@zhongnansu zhongnansu Oct 6, 2022

Choose a reason for hiding this comment

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

10000 is a commonly used value in the OpenSearch/osd world, it's the default max OS search hits, see reference here. Tho it's not a good practice to use hardcode, but sadly this value is used everywhere in the codebase. My suggestion is we can add an OSD global config such as savedObjects.PaginationSize, it could be tracked as a new issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know 😊

@kristenTian kristenTian merged commit 4095e12 into opensearch-project:main Oct 6, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2505-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4095e123d77182bed213fedf41be00ccaffd34a7
# Push it to GitHub
git push --set-upstream origin backport/backport-2505-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

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

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2505-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4095e123d77182bed213fedf41be00ccaffd34a7
# Push it to GitHub
git push --set-upstream origin backport/backport-2505-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

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

zhongnansu pushed a commit to zhongnansu/OpenSearch-Dashboards that referenced this pull request Oct 6, 2022
…2505)

Signed-off-by: Kristen Tian <[email protected]>

Signed-off-by: Kristen Tian <[email protected]>
(cherry picked from commit 4095e12)
kavilla pushed a commit that referenced this pull request Oct 10, 2022
Signed-off-by: Kristen Tian <[email protected]>
(cherry picked from commit 4095e12)
@AMoo-Miki AMoo-Miki added enhancement New feature or request v2.4.0 'Issues and PRs related to version v2.4.0' labels Nov 5, 2022
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Dec 1, 2022
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Dec 1, 2022
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
…2505)

Signed-off-by: Kristen Tian <[email protected]>

Signed-off-by: Kristen Tian <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request multiple datasource multiple datasource project v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants