-
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
[Uptime] Fix chart wrapping for monitor page #49268
[Uptime] Fix chart wrapping for monitor page #49268
Conversation
Pinging @elastic/uptime (Team:uptime) |
2f8fbf7
to
63a2c6c
Compare
💚 Build Succeeded |
💚 Build Succeeded |
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.
Works great in tests and the code looks pretty good, but just to confirm, we've investigated and there's no way to make this responsive without custom styling. Is that correct? Should we open an EUI issue about this?
import styled from 'styled-components'; | ||
|
||
const ResponsiveWrapper = styled.div` | ||
margin-left: 120px; |
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.
So, is there no EUI way to get this to work in a responsive way?
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.
You can see this comment for more info about why we have this custom styling here. I can ask someone from EUI to review this as well to see if they have a better recommendation.
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.
For more clarification, we wanted to introduce additional whitespace to balance out the view a bit, and because there wasn't enough separation in the default mobile view. In essence we're still using EUI's styling, just customizing it slightly. I've asked the EUI team to give us feedback on this though.
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
Summary
Resolves #49243.
Extracts the responsive wrapping functionality we previously introduced to a separate component, and allows consuming code to choose between a responsive element or an
EuiPanel
.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers