-
Notifications
You must be signed in to change notification settings - Fork 885
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
[Console][Multiple Datasource]Fine tuned dev tool datasource selector UI #3806
[Console][Multiple Datasource]Fine tuned dev tool datasource selector UI #3806
Conversation
Signed-off-by: Su <szhongna@amazon.com>
147cc7b
to
af54dfa
Compare
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #3806 +/- ##
==========================================
- Coverage 66.42% 66.41% -0.01%
==========================================
Files 3209 3209
Lines 61730 61732 +2
Branches 9533 9533
==========================================
- Hits 41004 41002 -2
- Misses 18439 18442 +3
- Partials 2287 2288 +1
Flags with carried forward coverage won't be shown. Click here to find out more. see 3 files with indirect coverage changes 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.
A couple of clarifying questions, but this definitely looks better than the previous version.
.devAppTabs { | ||
display: flex; | ||
flex-flow: row wrap; | ||
justify-content: space-between; | ||
} |
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.
Can you explain more about why this wasn't achievable with EuiFlexGroup
and EuiFlexItem
as in the previous code? Because it likely means we need to open an issue in OUI to figure out how to address the need 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.
I don't think it's an issue with EuiFlexGroup
or EuiFlexItem
. Previously when we use FlexGroup
to have 2 flex item, one is EuiTabs
, one is datasourcePicker
, then there's this ugly line cut-off(comes with EuiTab). We want the line to stretch all the way to be under the DataSourcePicker
.
Reason for the cut-off is that we have 2 flexItem
. But we want it to look better to avoid the cut-off, and the solution in this PR is by making DataSourePicker
within EuiTab component, while removing the FlexGroup
and FlexItem
. But since EuiTabs
itself is not inherently a flex component, I need to achieve that in CSS.
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 looked into this a bit - I think that the fundamental issue is that the current usage or EuiTabs
here is not really the best component for the way we want the top of the page to work (EuiPageHeader
with tabs
and rightSideItems
is more of what we want. But I think it makes the most sense to go forward with this hack for now, and then refactor it a part of a larger effort to convert the dev_tools
and console
plugins to be fully built with OUI components.
…d on the dataSourceEnabled prop Signed-off-by: Su <szhongna@amazon.com>
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.
Screenshot with the new changes is looking good. There is a link check failure which is not related to this change.
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.
This looks a lot better for now. Will refactor as part of OUI-ification of dev_tools
and console
.
.devAppTabs { | ||
display: flex; | ||
flex-flow: row wrap; | ||
justify-content: space-between; | ||
} |
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 looked into this a bit - I think that the fundamental issue is that the current usage or EuiTabs
here is not really the best component for the way we want the top of the page to work (EuiPageHeader
with tabs
and rightSideItems
is more of what we want. But I think it makes the most sense to go forward with this hack for now, and then refactor it a part of a larger effort to convert the dev_tools
and console
plugins to be fully built with OUI components.
link checker failure can be ignored. Awaiting cypress re-run. |
… UI (#3806) * Fine tuned dev tool datasource selector UI * Refactor DevToolsWrapper to conditionally render the EuiComboBox based on the dataSourceEnabled prop Signed-off-by: Su <szhongna@amazon.com> --------- Signed-off-by: Su <szhongna@amazon.com> (cherry picked from commit 971165f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
… UI (#3806) (#3850) * Fine tuned dev tool datasource selector UI * Refactor DevToolsWrapper to conditionally render the EuiComboBox based on the dataSourceEnabled prop --------- (cherry picked from commit 971165f) Signed-off-by: Su <szhongna@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>
… UI (opensearch-project#3806) * Fine tuned dev tool datasource selector UI * Refactor DevToolsWrapper to conditionally render the EuiComboBox based on the dataSourceEnabled prop Signed-off-by: Su <szhongna@amazon.com> --------- Signed-off-by: Su <szhongna@amazon.com> Signed-off-by: David Sinclair <david@sinclair.tech>
Description
[Console][Multiple Datasource]Fine tuned dev tool datasource selector UI
Coming from @KrooshalUX 's feedback #3754 (comment). Tuned the UI to make it look nicer, while not breaking the pattern of other dev tool registered plugins
Issues Resolved
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr