[Infra UI] Replace node details flyout with asset details flyout in the inventory page #166965
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…with-asset-details-flyout-in-the-inventory-page
jennypavlova
commented
Sep 22, 2023
| return source ? ( | ||
| <AssetDetails | ||
| asset={{ id: assetName, name: assetName }} | ||
| assetType="host" |
There was a problem hiding this comment.
This will be replaced when more asset types are supported
There was a problem hiding this comment.
Should we already make it accept assetType as prop?
|
|
||
| after(async () => { | ||
| await retry.try(async () => { | ||
| await pageObjects.infraHome.closeFlyout(); |
There was a problem hiding this comment.
This is not in the main block because after the next test when the user is back from the APM page (next test) he won't see the flyout because the inventory state is reset and the host is not available
| @@ -118,6 +126,92 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => { | |||
| await pageObjects.infraHome.getWaffleMap(); | |||
| // await pageObjects.infraHome.getWaffleMapTooltips(); see https://github.com/elastic/kibana/issues/137903 | |||
| }); | |||
|
|
|||
| describe('Asset Details flyout', () => { | |||
There was a problem hiding this comment.
I know that we have tests on the host view here I tried to mostly cover the differences
jennypavlova
added
Team:Infra Monitoring UI - DEPRECATED
|
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
…with-asset-details-flyout-in-the-inventory-page
…with-asset-details-flyout-in-the-inventory-page
…with-asset-details-flyout-in-the-inventory-page
|
@elasticmachine merge upstream |
…with-asset-details-flyout-in-the-inventory-page
…with-asset-details-flyout-in-the-inventory-page
…with-asset-details-flyout-in-the-inventory-page
jennypavlova
changed the title
jennypavlova
force-pushed
the
161754-infra-ui-replace-node-details-flyout-with-asset-details-flyout-in-the-inventory-page
branch
from
d7f6f32 to
c1364b9
Compare
jennypavlova
force-pushed
the
161754-infra-ui-replace-node-details-flyout-with-asset-details-flyout-in-the-inventory-page
branch
from
c1364b9 to
a9308ab
Compare
| @@ -112,15 +112,14 @@ export const NodeSquare = ({ | |||
| color: string; | |||
| nodeName: string; | |||
| value: string; | |||
| isOverlayOpen: boolean; | |||
| nodeBorder?: CSSProperties; | |||
There was a problem hiding this comment.
CSSProperties makes this prop accept any style attributes, not only border. So nothing prevents it from receiving { padding-top: "10px" }, for instance.
Would this work?
export const NodeSquare = ({
...
showBorder = false
}: {
...,
showBorder?: boolean
}) => {
const style: CSSProperties | undefined = showBorder ? { border: 'solid 4px #000' } : undefined
...
<SquareOuter color={color} style={style}>then to use it:
const nodeSquare = (
<NodeSquare
squareSize={squareSize}
color={color}
nodeName={node.name}
value={value}
showBorder={detailsItemId === node.name || isPopoverOpen}
/>
);There was a problem hiding this comment.
Good point, thanks :) I changed it as suggested
| }, | ||
| alertRule: { | ||
| onCreateRuleClick: () => setIsAlertFlyoutVisible(true), | ||
| inventoryRuleLabel: i18n.translate('xpack.infra.infra.nodeDetails.createAlertLink', { |
There was a problem hiding this comment.
Both open the alert flyout for the Inventory Threshold rule, this one at the top might not be needed anymore.
I understand that this is what we have in the old flyout, but I think it will be enough if we just have the create rule button in the Alerts section.
@roshan-elastic @jennypavlova , wdyt?
…with-asset-details-flyout-in-the-inventory-page
…with-asset-details-flyout-in-the-inventory-page
crespocarlos
approved these changes
Sep 27, 2023
| `; | ||
| const styles = { |
There was a problem hiding this comment.
Thanks for replacing the euiStyled
| @@ -0,0 +1,165 @@ | |||
| /* | |||
There was a problem hiding this comment.
looks good to me! If in the future we find a better way, we can refactor it.
Comment on lines
+124
to
+125
| onKeyPress={togglePopover} | ||
| onFocus={showToolTip} |
There was a problem hiding this comment.
Why do we need these?
There was a problem hiding this comment.
It was an accessibility eslint error: onMouseOver must be accompanied by onFocus for accessibility.eslint jsx-a11y/mouse-events-have-key-events
|
@elasticmachine merge upstream |
…with-asset-details-flyout-in-the-inventory-page
…with-asset-details-flyout-in-the-inventory-page
jennypavlova
force-pushed
the
161754-infra-ui-replace-node-details-flyout-with-asset-details-flyout-in-the-inventory-page
branch
from
b2be471 to
07e47c2
Compare
…-replace-node-details-flyout-with-asset-details-flyout-in-the-inventory-page
|
Hey @crespocarlos,
I removed it as discussed. One thing we need to do is allow the current create rule component to accept additional PS. Issue added #167524 |
…with-asset-details-flyout-in-the-inventory-page
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
jennypavlova
added a commit
that referenced
this pull request
Oct 6, 2023
Closes #168156 ## Summary This PR fixes process charts error:  I assume that after the merge of the refactoring PR, those files were recreated by another PR or most likely when `main` was merged to [the refactoring PR](#166965) with them as resolved conflicts. Deleting the files again fixed the issue. ## Testing I found the issue on serverless but it probably can be reproduced on on-prem as well On Serverless: - Start a local es instance: `yarn es serverless --kill --clean --license trial --ssl` - Enable infra in the `serverless.oblt.dev.yml` file: - `xpack.infra.enabled: true` - Start a local kibana instance: `yarn serverless-oblt --ssl` and see if the side nav contains the Infrastructure item - Navigate to https://0.0.0.0:5601/ftw/app/hosts and open Asset details flyout or page - Click on the process tab and expand one of the processes - The charts (CPU and Memory) should be visible
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 6, 2023
Closes elastic#168156 ## Summary This PR fixes process charts error:  I assume that after the merge of the refactoring PR, those files were recreated by another PR or most likely when `main` was merged to [the refactoring PR](elastic#166965) with them as resolved conflicts. Deleting the files again fixed the issue. ## Testing I found the issue on serverless but it probably can be reproduced on on-prem as well On Serverless: - Start a local es instance: `yarn es serverless --kill --clean --license trial --ssl` - Enable infra in the `serverless.oblt.dev.yml` file: - `xpack.infra.enabled: true` - Start a local kibana instance: `yarn serverless-oblt --ssl` and see if the side nav contains the Infrastructure item - Navigate to https://0.0.0.0:5601/ftw/app/hosts and open Asset details flyout or page - Click on the process tab and expand one of the processes - The charts (CPU and Memory) should be visible (cherry picked from commit 000b70c)
kibanamachine
added a commit
that referenced
this pull request
Oct 6, 2023
…8210) # Backport This will backport the following commits from `main` to `8.11`: - [[Infra UI] Fix process charts after refactoring (#168159)](#168159) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"jennypavlova","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-10-06T12:01:19Z","message":"[Infra UI] Fix process charts after refactoring (#168159)\n\nCloses #168156 \r\n## Summary\r\n\r\nThis PR fixes process charts error: \r\n\r\n\r\n\r\nI assume that after the merge of the refactoring PR, those files were\r\nrecreated by another PR or most likely when `main` was merged to [the\r\nrefactoring PR](#166965) with them\r\nas resolved conflicts. Deleting the files again fixed the issue.\r\n\r\n## Testing\r\nI found the issue on serverless but it probably can be reproduced on\r\non-prem as well\r\nOn Serverless: \r\n- Start a local es instance: `yarn es serverless --kill --clean\r\n--license trial --ssl`\r\n- Enable infra in the `serverless.oblt.dev.yml` file:\r\n - `xpack.infra.enabled: true`\r\n- Start a local kibana instance: `yarn serverless-oblt --ssl` and see if\r\nthe side nav contains the Infrastructure item\r\n- Navigate to https://0.0.0.0:5601/ftw/app/hosts and open Asset details\r\nflyout or page\r\n- Click on the process tab and expand one of the processes\r\n- The charts (CPU and Memory) should be visible","sha":"000b70c000ec1c8ce9d2ccda6faf60247116a263","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Infra Monitoring UI","release_note:skip","backport:prev-minor","v8.12.0"],"number":168159,"url":"https://github.com/elastic/kibana/pull/168159","mergeCommit":{"message":"[Infra UI] Fix process charts after refactoring (#168159)\n\nCloses #168156 \r\n## Summary\r\n\r\nThis PR fixes process charts error: \r\n\r\n\r\n\r\nI assume that after the merge of the refactoring PR, those files were\r\nrecreated by another PR or most likely when `main` was merged to [the\r\nrefactoring PR](#166965) with them\r\nas resolved conflicts. Deleting the files again fixed the issue.\r\n\r\n## Testing\r\nI found the issue on serverless but it probably can be reproduced on\r\non-prem as well\r\nOn Serverless: \r\n- Start a local es instance: `yarn es serverless --kill --clean\r\n--license trial --ssl`\r\n- Enable infra in the `serverless.oblt.dev.yml` file:\r\n - `xpack.infra.enabled: true`\r\n- Start a local kibana instance: `yarn serverless-oblt --ssl` and see if\r\nthe side nav contains the Infrastructure item\r\n- Navigate to https://0.0.0.0:5601/ftw/app/hosts and open Asset details\r\nflyout or page\r\n- Click on the process tab and expand one of the processes\r\n- The charts (CPU and Memory) should be visible","sha":"000b70c000ec1c8ce9d2ccda6faf60247116a263"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/168159","number":168159,"mergeCommit":{"message":"[Infra UI] Fix process charts after refactoring (#168159)\n\nCloses #168156 \r\n## Summary\r\n\r\nThis PR fixes process charts error: \r\n\r\n\r\n\r\nI assume that after the merge of the refactoring PR, those files were\r\nrecreated by another PR or most likely when `main` was merged to [the\r\nrefactoring PR](#166965) with them\r\nas resolved conflicts. Deleting the files again fixed the issue.\r\n\r\n## Testing\r\nI found the issue on serverless but it probably can be reproduced on\r\non-prem as well\r\nOn Serverless: \r\n- Start a local es instance: `yarn es serverless --kill --clean\r\n--license trial --ssl`\r\n- Enable infra in the `serverless.oblt.dev.yml` file:\r\n - `xpack.infra.enabled: true`\r\n- Start a local kibana instance: `yarn serverless-oblt --ssl` and see if\r\nthe side nav contains the Infrastructure item\r\n- Navigate to https://0.0.0.0:5601/ftw/app/hosts and open Asset details\r\nflyout or page\r\n- Click on the process tab and expand one of the processes\r\n- The charts (CPU and Memory) should be visible","sha":"000b70c000ec1c8ce9d2ccda6faf60247116a263"}}]}] BACKPORT--> Co-authored-by: jennypavlova <[email protected]>
dej611
pushed a commit
to dej611/kibana
that referenced
this pull request
Oct 17, 2023
Closes elastic#168156 ## Summary This PR fixes process charts error:  I assume that after the merge of the refactoring PR, those files were recreated by another PR or most likely when `main` was merged to [the refactoring PR](elastic#166965) with them as resolved conflicts. Deleting the files again fixed the issue. ## Testing I found the issue on serverless but it probably can be reproduced on on-prem as well On Serverless: - Start a local es instance: `yarn es serverless --kill --clean --license trial --ssl` - Enable infra in the `serverless.oblt.dev.yml` file: - `xpack.infra.enabled: true` - Start a local kibana instance: `yarn serverless-oblt --ssl` and see if the side nav contains the Infrastructure item - Navigate to https://0.0.0.0:5601/ftw/app/hosts and open Asset details flyout or page - Click on the process tab and expand one of the processes - The charts (CPU and Memory) should be visible
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #161754
Closes #166807
To make the testing and review easier I merged the old components cleanup PR into this one
Summary
This PR replaces the old node details view with the asset details flyout
Old
New
Testing
Go to inventory
Click on a host in the waffle map
Click on any host
- These changes are related only if a
Hostis selected- in the case of a pod the view shouldn't be changed:Check the new flyout functionality
inventory_asset_details.mov
Note: the selected host should have a border like in the previous version (this I fixed in the last commit) so it should be added if there is a selected node:
