-
Notifications
You must be signed in to change notification settings - Fork 916
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] skip data source view when pick default #2574
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2574 +/- ##
=======================================
Coverage 66.81% 66.81%
=======================================
Files 3207 3207
Lines 61136 61136
Branches 9313 9313
=======================================
+ Hits 40845 40849 +4
+ Misses 18059 18056 -3
+ Partials 2232 2231 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update PR description. Add the issues you resolved. For UI changes, better to have screenshots/gif/videos attached
CHANGELOG.md
Outdated
@@ -22,6 +22,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
* [Multi DataSource] Add experimental callout for index pattern section ([#2523](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2523)) | |||
* [Multi DataSource] Add data source config to opensearch-dashboards-docker ([#2557](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2557)) | |||
* [Multi DataSource] Make text content dynamically translated & update unit tests ([#2570](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2570)) | |||
* [Multi DataSource] Skip data source view in index pattern step when pick default ([#2574](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2574)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a bug fix, can you add to the bug section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to that, but not sure what should be categorized as bug fix, given this feature has not been released.
@@ -103,14 +103,14 @@ export const Header: React.FC<HeaderProps> = ({ | |||
<EuiFlexGroup> | |||
<EuiFlexItem> | |||
<EuiForm isInvalid={isInputInvalid}> | |||
{dataSourceEnabled | |||
{dataSourceEnabled && dataSourceRef?.title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we only check if the dataSourceRef
is defined, instead of using title. Is there a case that dataSouceRef comes without title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not today, but not sure in future, that's why I prefer to do the safer check -- also the UX only uses the title section and it does not make sense to show data source when it's not presented.
There are no UX and issue related to this. Is the process require we have to create an issue every time we submit a PR? |
You can create one if not exists. Yes, it's required. Only exception might be spelling error. See https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/CONTRIBUTING.md#first-things-first |
Thanks, "If it's truly a trivial change (e.g. spelling error), you can skip this step " I think this pr categorize as a trivial change lol |
Signed-off-by: Kristen Tian <[email protected]>
I am holding my opinion on if this is a trivial change. Since the code will change the user experience from what's current in |
Signed-off-by: Kristen Tian <[email protected]> Signed-off-by: Kristen Tian <[email protected]> (cherry picked from commit 66f07c3)
This PR does not change any user experience or logic, it is only a one line tweak so that the previous commit can function as expected. -- which has issues already created and linked from UX. -- Speaking of which, I could link here: opensearch-project/ux#42 |
Signed-off-by: Kristen Tian <[email protected]> Signed-off-by: Kristen Tian <[email protected]> (cherry picked from commit 66f07c3) Co-authored-by: Kristen Tian <[email protected]>
Signed-off-by: Kristen Tian <[email protected]> Signed-off-by: Kristen Tian <[email protected]> Signed-off-by: Sergey V. Osipov <[email protected]>
Signed-off-by: Kristen Tian [email protected]
Description
Skip data source view when pick default - When OSD enable data source feature, user can still choose to use default cluster, in this case, index pattern creation should render with previous experience for this phase.
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr