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

Update Discover appearance #8651

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

AMoo-Miki
Copy link
Collaborator

Description

Update Discover appearance

Screenshot

Before

old l old p

After: with home:useNewHomePage & query:enhancements:enabled

new l new p

After:

query:enhancements:enabled: false home:useNewHomePage: false
no enh legacy
--- ---

After: darkMode: true

new pd ---

Testing the changes

Changelog

  • feat: Update the appearance of Discover

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

@ashwin-pc
Copy link
Member

@AMoo-Miki i dont think the styles should affect the UI if the navbar changes are turned off. That beign said if its consistent with the rest of the app when these flags are turned off, that should be fine.

@AMoo-Miki
Copy link
Collaborator Author

@AMoo-Miki i dont think the styles should affect the UI if the navbar changes are turned off. That beign said if its consistent with the rest of the app when these flags are turned off, that should be fine.

These changes were irrespective of theme version or color-scheme; they all use OUI variables or default component props and I wanted to show that things look cohesive across feature flags.

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.86%. Comparing base (923cce8) to head (8b171b6).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...ta/public/ui/dataset_selector/dataset_selector.tsx 60.00% 1 Missing and 1 partial ⚠️
...ugins/data/public/ui/filter_bar/filter_options.tsx 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8651      +/-   ##
==========================================
- Coverage   60.87%   60.86%   -0.01%     
==========================================
  Files        3790     3790              
  Lines       90223    90243      +20     
  Branches    14143    14147       +4     
==========================================
+ Hits        54921    54930       +9     
- Misses      31838    31845       +7     
- Partials     3464     3468       +4     
Flag Coverage Δ
Linux_1 29.08% <33.33%> (+<0.01%) ⬆️
Linux_2 56.39% <ø> (ø)
Linux_3 37.71% <57.14%> (-0.01%) ⬇️
Linux_4 29.80% <33.33%> (+<0.01%) ⬆️
Windows_1 29.09% <33.33%> (+<0.01%) ⬆️
Windows_2 56.34% <ø> (ø)
Windows_3 37.72% <57.14%> (-0.01%) ⬇️
Windows_4 29.80% <33.33%> (+<0.01%) ⬆️

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.

@kgcreative
Copy link
Member

Visual changes LGTM

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.

main concern is on if we're making discover table less dense, but assume that's what design wanted so not blocking (does go against what we agreed upon earlier)

@@ -1,9 +1,9 @@
.context-nav-wrapper {
border: none !important;
border-top-right-radius: $euiSizeL;
border-bottom-right-radius: $euiSizeL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ty!

@@ -43,14 +43,15 @@ export const DefaultInput: React.FC<DefaultInputProps> = ({
options={{
minimap: { enabled: false },
scrollBeyondLastLine: false,
fontSize: 14,
fontFamily: 'Roboto Mono',
Copy link
Collaborator

Choose a reason for hiding this comment

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

ty

@@ -4,7 +4,7 @@

// nested for specificity
.docTableHeaderField {
padding: calc($euiSizeM / 2); // corresponds to DataGrid medium cellPadding
padding: $euiSizeS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

callout: this was intentionally lowered for density. not going to block on this because i'm not sure why we're making this change, but preference was still for this to be denser, similar to competitor data tables spacing

also, haven't gone through this entire cr yet, but double check the hover behavior on the cells as there was some change to that after adjusting density here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked with UX and the change is good to stay.

@@ -27,7 +48,8 @@
.euiButtonEmpty__content {
font-size: $euiFontSizeXS;
font-weight: $euiFontWeightSemiBold;
justify-content: flex-end;
justify-content: flex-start;
padding-left: $euiSizeM + 2px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why 2px?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The design had a 2px spacing between the section headers and the elements below them.

@@ -9,7 +9,7 @@
}

.osdTopNavMenuGroupedActions {
background-color: $euiColorEmptyShade;
background-color: $euiPageBackgroundColor;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: think you could just make this transparent to get buttons to be consistent with how buttons normally behave in case their styling changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Will do that in a subsequent PR.

@@ -88,7 +88,7 @@ $osdDocTableCellPadding: calc($euiSizeM / 2); // corresponds to DataGrid medium
// nested for specificity
.osd-table {
.osdDocTableCell {
padding: $osdDocTableCellPadding;
padding: $euiSizeS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this go from 6 -> 8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked with UX and Viraj; the change is good to stay.

@AMoo-Miki AMoo-Miki merged commit 17103ba into opensearch-project:main Oct 18, 2024
75 of 76 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-8651-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 17103ba86370456fa5df6ccc722a22bee8fe2d35
# Push it to GitHub
git push --set-upstream origin backport/backport-8651-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-8651-to-2.x.

AMoo-Miki added a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Oct 18, 2024
* Update Discover appearance

Signed-off-by: Miki <[email protected]>

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

---------

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

(cherry picked from commit 17103ba)
Signed-off-by: Miki <[email protected]>
AMoo-Miki added a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Oct 19, 2024
* Update Discover appearance

Signed-off-by: Miki <[email protected]>

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

---------

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

(cherry picked from commit 17103ba)
Signed-off-by: Miki <[email protected]>
amsiglan pushed a commit to amsiglan/OpenSearch-Dashboards that referenced this pull request Oct 19, 2024
* Update Discover appearance

Signed-off-by: Miki <[email protected]>

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

---------

Signed-off-by: Miki <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
AMoo-Miki added a commit that referenced this pull request Oct 19, 2024
…o Results Page #8616  (#8663)

* Update Discover appearance (#8651)

* Update Discover appearance

Signed-off-by: Miki <[email protected]>

* Changeset file for PR #8651 created/updated

---------

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

(cherry picked from commit 17103ba)
Signed-off-by: Miki <[email protected]>

* Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View (#8613)

* Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View

This PR primarily addresses the scenario when no index patterns (general) is available in the Discover view.
Instead of redirecting users to the index management page, it introduces a new "No Index Patterns" panel.
This panel provides users with the option to open a data selector and add index patterns
directly from the Discover view, improving the user experience for new or empty deployments.

To achieve, we move the selectedDataset state from ConnectedDatasetSelector to the app container's
state management. This allows the AdvancedSelector, opened from the AppContainer, to update
the dataset state effectively. Key changes include:

* Implementing NoIndexPatternsPanel and AdvancedSelector components.
* Refactoring dataset state management in AppContainer and Sidebar.
* Modifying DiscoverCanvas to conditionally render NoIndexPatternsPanel.
* Updating ConnectedDatasetSelector to use shared state and dataset change handling.

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

* Update design of no data selected

Signed-off-by: Miki <[email protected]>

* use i18n

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

* fix comments

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

* Update design of no data selected

Signed-off-by: Miki <[email protected]>

* fix lint error

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

---------

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

(cherry picked from commit 6659139)
Signed-off-by: Miki <[email protected]>

* [Discover]Sample Queries and Saved Queries in No Results Page (#8616)

* Sample Queries and Saved Queries in No Results Page

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

* Changeset file for PR #8616 created/updated

* Update styling

Signed-off-by: Miki <[email protected]>

---------

Signed-off-by: Sean Li <[email protected]>
Signed-off-by: Miki <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Co-authored-by: Miki <[email protected]>
(cherry picked from commit 9da1b77)

---------

Signed-off-by: Miki <[email protected]>
Signed-off-by: Anan Zhuang <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Sean Li <[email protected]>
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Oct 19, 2024
Also:
* Fix some React errors

Signed-off-by: Miki <[email protected]>

---------

Fix random big number during loading in query editor result (opensearch-project#8650)

* Fix random big number during loading in query editor result

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

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

* Fix initial loading number

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit a7414f0)

---------

[bug] address different issues with dataset selector (opensearch-project#8665)

* [bug] address some issues with dataset selector

Moved the dataset selector back to the search bar and access it by ref in the sidebar.

Avoid out of sync issue.
* update the logic for ensuring index pattern
* dont add ability to open button
* empty state but missing data set selector button
* fix tests
* add back styling

Signed-off-by: Kawika Avilla <[email protected]>
(cherry picked from commit e23f332)

---------

[bug] Discover UI stuck on searching after deleting index pattern (opensearch-project#8659)

* [bug] Discover UI stuck on searching after deleting index pattern

When using Discover with query enhancement enabled, deleting an index pattern from Index Management does not properly update the "Recently selected data" list in Discover. This causes the UI to become stuck in a "Searching" state when attempting to use
Discover after deleting an index pattern.

Handle the error case where the use index patterns hook caught error when
enhancements was enabled.

issue resolved:
opensearch-project#8612

(cherry picked from commit 4808094)

---------

[Discover]Sample Queries and Saved Queries in No Results Page (opensearch-project#8616)

* Sample Queries and Saved Queries in No Results Page

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

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

* Update styling

Signed-off-by: Miki <[email protected]>

(cherry picked from commit 9da1b77)

---------

Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View (opensearch-project#8613)

* Improve Empty State Handling: Add No Index Patterns Panel with Data Selection in Discover View

This PR primarily addresses the scenario when no index patterns (general) is available in the Discover view.
Instead of redirecting users to the index management page, it introduces a new "No Index Patterns" panel.
This panel provides users with the option to open a data selector and add index patterns
directly from the Discover view, improving the user experience for new or empty deployments.

To achieve, we move the selectedDataset state from ConnectedDatasetSelector to the app container's
state management. This allows the AdvancedSelector, opened from the AppContainer, to update
the dataset state effectively. Key changes include:

* Implementing NoIndexPatternsPanel and AdvancedSelector components.
* Refactoring dataset state management in AppContainer and Sidebar.
* Modifying DiscoverCanvas to conditionally render NoIndexPatternsPanel.
* Updating ConnectedDatasetSelector to use shared state and dataset change handling.

* Update design of no data selected
* use i18n
* fix comments
* Update design of no data selected
* fix lint error

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

(cherry picked from commit 6659139)

---------

Update Discover appearance (opensearch-project#8651)

* Update Discover appearance

Signed-off-by: Miki <[email protected]>

(cherry picked from commit 17103ba)

---------

Move DatasetSelector from data plugin queryString comp to DataExplorer (opensearch-project#8598)

* Move DatasetSelector to DataExplorer
* Style Disover after moving DatasetSelector to DataExplorer
* fix the test by adding the getUpdates$ method to the mock queryString object

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

(cherry picked from commit 923cce8)
@AMoo-Miki
Copy link
Collaborator Author

Manually backported to 2.x with #8670

ashwin-pc pushed a commit to ashwin-pc/OpenSearch-Dashboards that referenced this pull request Oct 19, 2024
* Update Discover appearance

Signed-off-by: Miki <[email protected]>

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

---------

Signed-off-by: Miki <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[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.

5 participants