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.

Sign up for GitHub

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

Jump to bottom

[Multiple Datasource] Pass data source menu to top nav via function instead of using component #6594

Merged
merged 2 commits into from
Apr 22, 2024
Merged

[Multiple Datasource] Pass data source menu to top nav via function instead of using component #6594

merged 2 commits into from
Apr 22, 2024

Conversation

BionIT
Copy link
Collaborator

@BionIT BionIT commented Apr 22, 2024 •
edited
Loading

Description

In this change, we changed to pass data source menu to top nav via function instead of using the component, this way we avoid coupling the plugin level props into the top nav menu component

Issues Resolved

Screenshot

topnavmount.mp4

Testing the changes

The following steps were performed in the recording:

  1. mount data source list all view along with the topnav menu, will see the component mounted, default data source is shown, no local cluster is showing since hide local cluster setting is true
  2. mount data source list active view along with the topnav menu, will see the component mounted, default data source is shown , no local cluster is showing since hide local cluster setting is true
  3. mount data source single readonly along with the topnav menu, will see the component mounted, default data source is shown , no local cluster is showing since hide local cluster setting is true
  4. mount data source single selectable along with the topnav menu, will see the component mounted, default data source is shown , no local cluster is showing since hide local cluster setting is true

Screenshot when hide local cluster is true:
Screenshot 2024-04-22 at 11 53 53 AM

Screenshot when hide local cluster is false:
Screenshot 2024-04-22 at 11 53 30 AM

Changelog

  • skip

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

@BionIT
change top nav to use interface to render data source menu
84d682b 
Signed-off-by: Lu Yu <nluyu@amazon.com>
@BionIT
change value
72e12c5 
Signed-off-by: Lu Yu <nluyu@amazon.com>
@github-actions GitHub Actions
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.

@github-actions github-actions bot added failed changeset Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Apr 22, 2024
zhongnansu
zhongnansu reviewed Apr 22, 2024
View reviewed changes
...s/data_source_management/public/components/data_source_menu/create_data_source_menu.test.tsx
);
spyOn(utils, 'getApplication').and.returnValue({ id: 'test2' });
spyOn(utils, 'getUiSettings').and.returnValue(uiSettings);
spyOn(utils, 'getHideLocalCluster').and.returnValue({ enabled: true });
Copy link
Member

Choose a reason for hiding this comment

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

should we set this to enabled: false based on the test case description?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zhongnansu This test itself needs to be fixed since it doesn't change disregards of local cluster is set since it doesn't check the options from popover. We can cut another meta issue for all the tests that we are missing testing, or do you think this needs to block the change?

Copy link
Member

Choose a reason for hiding this comment

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

@BionIT agreed that we should fix the text, this is non blocking issue
I think in this PR, the test was modified to remove the check for local cluster existence
https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6358/files#diff-ff98db435bf1f4c1bfe7ea1869b5d25b7e731148ce6060333bf37b0dee33219fL78

@BionIT BionIT merged commit b7b27aa into opensearch-project:main Apr 22, 2024
68 of 69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 22, 2024
@github-actions
[Multiple Datasource] Pass data source menu to top nav via function i…
b52e72b 
…nstead of using component (#6594)

* change top nav to use interface to render data source menu

Signed-off-by: Lu Yu <nluyu@amazon.com>

* change value

Signed-off-by: Lu Yu <nluyu@amazon.com>

---------

Signed-off-by: Lu Yu <nluyu@amazon.com>
(cherry picked from commit b7b27aa)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Apr 22, 2024
Merged
@BionIT BionIT mentioned this pull request Apr 22, 2024
Closed
77 tasks
BionIT pushed a commit that referenced this pull request Apr 22, 2024
@opensearch-trigger-bot @github-actions
[Multiple Datasource] Pass data source menu to top nav via function i…
accdcf2 
…nstead of using component (#6594) (#6601)

* change top nav to use interface to render data source menu



* change value



---------


(cherry picked from commit b7b27aa)

Signed-off-by: Lu Yu <nluyu@amazon.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>
@BionIT BionIT mentioned this pull request Apr 22, 2024
Closed
36 tasks
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 3, 2024
@BionIT @LDrago27
[Multiple Datasource] Pass data source menu to top nav via function i…
2129aaf 
…nstead of using component (opensearch-project#6594)

* change top nav to use interface to render data source menu

Signed-off-by: Lu Yu <nluyu@amazon.com>

* change value

Signed-off-by: Lu Yu <nluyu@amazon.com>

---------

Signed-off-by: Lu Yu <nluyu@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhongnansu zhongnansu zhongnansu approved these changes

@Flyingliuhub Flyingliuhub Flyingliuhub approved these changes

@ananzh ananzh Awaiting requested review from ananzh ananzh is a code owner

@kavilla kavilla Awaiting requested review from kavilla kavilla is a code owner

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@joshuarrrr joshuarrrr Awaiting requested review from joshuarrrr joshuarrrr is a code owner

@abbyhu2000 abbyhu2000 Awaiting requested review from abbyhu2000 abbyhu2000 is a code owner

@zengyan-amazon zengyan-amazon Awaiting requested review from zengyan-amazon zengyan-amazon is a code owner

@manasvinibs manasvinibs Awaiting requested review from manasvinibs manasvinibs is a code owner

@ZilongX ZilongX Awaiting requested review from ZilongX ZilongX is a code owner

@curq curq Awaiting requested review from curq curq is a code owner

@bandinib-amzn bandinib-amzn Awaiting requested review from bandinib-amzn bandinib-amzn is a code owner

@SuZhou-Joe SuZhou-Joe Awaiting requested review from SuZhou-Joe SuZhou-Joe is a code owner

@ruanyl ruanyl Awaiting requested review from ruanyl ruanyl is a code owner

@xinruiba xinruiba Awaiting requested review from xinruiba xinruiba is a code owner

Assignees
No one assigned
Labels
all-star-contributor backport 2.x Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BionIT @zhongnansu @Flyingliuhub