-
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
APM header changes #82870
APM header changes #82870
Conversation
- Make APM and UX headers size medium instead of large - Remove margin around APM main container - Make APM tabs condensed - Switch environment filter and date picker positions - Move search bar (kuery + date picker) below the tabs - Wrap pages in `EuiPage` components - Set a minimum width on the enironment selector so it doesn't collapse when loading - Don't show search bar on service map Fixes elastic#81954.
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/uptime (Team:uptime) |
@elasticmachine merge upstream |
retest |
@elasticmachine merge upstream |
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.
Minor suggestion to change the padding of the tabs.
// be the primary color. | ||
const StyledTabs = styled(EuiTabs)` | ||
padding: ${({ theme }) => | ||
`${theme.eui.gutterTypes.gutterMedium} ${theme.eui.gutterTypes.gutterLarge}`}; |
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.
Another issue, wasn't sure where to comment on this within the code, but there's some focus states on the tabs that don't work as expected. I think Logs and Metrics have a larger focus area on the tabs than what's in the current tabs. APM Logs Lastly, the order of the selected service changed so now Metrics are at the end of the list. I'm assuming this was not intentional, just wanted to point it out. |
I can change this, but it makes the padding inconsistent with the tabs on the metrics, logs, security, and fleet pages.
I'll look into this.
It was intentional, but I can move it back. Since the tab usually is rendered later I found it less jarring if it gets added at the end than if it gets added between tabs. |
This is because we have I left these as-is so I wouldn't have to rework all of these (the way it's set up it's easy to get a link element but harder to just get the href.) The only difference I noticed before is that we had to override the link color so they weren't all blue, since this affects the focus I'll go ahead and fix this. |
@formgeist all of your comments should be addressed now. @sqren I had to go and implement |
OK, I will follow up with @elastic/observability-design to make the padding in the header consistent.
Alright - would be great to have this called out in the PR next time otherwise it might have gone unnoticed. I understand that the Metrics tab will mostly load in late because of the agent check, but with RUM moving to the UX app (where we'd remove the option), would we be able to assume that the service navigation should always show Metrics, rather than wait for the check? I might even say we can disable the option for RUM rather than hide it. We can move this discussion and decision to an issue. Also in that discussion, we can talk about whether the horizontal list should be different. cc @nehaduggal @sqren Thanks for addressing it - let's get this merged in and make polish changes in a later iteration. |
|
||
function ServiceInventoryLink(props: APMLinkExtendProps) { | ||
export function useServiceInventoryHref() { |
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.
Should the filename be changed to use_service_inventory_link.tsx
?
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 opened #83196 to look at this.
export function useServiceOverviewHref(serviceName: string) { | ||
return useAPMHref(`/services/${serviceName}/overview`); | ||
} | ||
|
||
export function ServiceOverviewLink({ |
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.
Is this component still needed if we have the hook? I'm all for simplifying our link helpers and having one way to do it might be better.
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 opened #83196 to look at this.
Sorry about that. I meant to put it in the description but missed it. |
@elasticmachine merge upstream |
retest |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
- Make APM and UX headers size medium instead of large - Remove margin around APM main container - Make APM tabs condensed - Switch environment filter and date picker positions - Move search bar (kuery + date picker) below the tabs - Wrap pages in `EuiPage` components - Set a minimum width on the enironment selector so it doesn't collapse when loading - Don't show search bar on service map Fixes elastic#81954.
- Make APM and UX headers size medium instead of large - Remove margin around APM main container - Make APM tabs condensed - Switch environment filter and date picker positions - Move search bar (kuery + date picker) below the tabs - Wrap pages in `EuiPage` components - Set a minimum width on the enironment selector so it doesn't collapse when loading - Don't show search bar on service map Fixes #81954.
* master: [Security Solution][Detections] Adds framework for replacing API schemas (elastic#82462) [Enterprise Search] Share Loading component (elastic#83246) Consolidates Jest configuration files and scripts (elastic#82671) APM header changes (elastic#82870) [Security Solutions] Adds a default for indicator match custom query of *:* (elastic#81727) [Security Solution] Note 10k object paging limit on Endpoint list (elastic#82889) [packerCache] fix gulp usage, don't archive node_modules (elastic#83327) Upgrade Node.js to version 12 (elastic#61587) [Actions] Removing placeholders and updating validation messages on connector forms (elastic#82734) [Fleet] Rename ingest_manager_api_integration tests fleet_api_integration (elastic#83011) [DOCS] Updates Discover docs (elastic#82773) [ML] Data frame analytics: Adds map view (elastic#81666) enables actions scoped within the stack to register at Basic license (elastic#82931)
This was supposed to be part of elastic#82870 but was missed. Add the search bar, format the heading stats, and wrape the page contents in an `EuiPage`.
EuiPage
componentsFixes #81954. Fixes #81723.
Before
After