-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Infrastructure UI] Asset Details: Add pins to the metadata table #161074
[Infrastructure UI] Asset Details: Add pins to the metadata table #161074
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
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.
Good job, Jenny 👏 ! This is very useful. Just left a few minor comments.
const handleAddPin = (pin: Field['name']) => { | ||
setPinnedItems([...pinnedItems, pin]); | ||
}; | ||
|
||
const handleRemovePin = (pin: Field['name']) => { | ||
if (pinnedItems && pinnedItems.includes(pin)) { | ||
setPinnedItems((pinnedItems ?? []).filter((pinName: string) => pin !== pinName)); | ||
} | ||
}; |
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.
These handlers are receiving pin
as parameter. When they are called, we basically pass fieldName
. I think we could do this instead
const handleAddPin = (pin: Field['name']) => { | |
setPinnedItems([...pinnedItems, pin]); | |
}; | |
const handleRemovePin = (pin: Field['name']) => { | |
if (pinnedItems && pinnedItems.includes(pin)) { | |
setPinnedItems((pinnedItems ?? []).filter((pinName: string) => pin !== pinName)); | |
} | |
}; | |
const handleAddPin = () => { | |
setPinnedItems([...pinnedItems, fieldName]); | |
}; | |
const handleRemovePin = () => { | |
if (pinnedItems && pinnedItems.includes(fieldName)) { | |
setPinnedItems((pinnedItems ?? []).filter((pinName: string) => fieldName !== pinName)); | |
} | |
}; |
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.
Good point, thanks! I change those 👍
export const AddMetadataPinToRow = ({ | ||
fieldName, | ||
pinnedItems, | ||
setPinnedItems, |
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.
Since this is an event handler, I'd name it onPinned
or something along those lines. Wdyt?
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.
Hmm this is basically setting the the pinned items to the locale storage here but in this component I can rename it 👍
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.
Renamed ✅
// FLAKY: https://github.com/elastic/kibana/issues/159368 | ||
// FLAKY: https://github.com/elastic/kibana/issues/159369 |
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 we're removing this in another PR, right?
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.
Yes, I removed them here as well :)
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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.
LGTM! Thanks for making the changes.
Closes #155190
Summary
This PR adds the possibility to pin different rows inside the metadata table in asset details embeddable. The pins are persisted in the local storage and should be available after refreshing/reopening the host flyout. The order and sorting are explained in this comment, so basically we keep the original sorting order of the table (
host
,cloud
,agent
) also for the pins.Testing
pins.mov