-
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
Fix management footer logo spacing bug #30654
Conversation
Pinging @elastic/kibana-app |
💔 Build Failed |
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.
The problem in IE is not the size of the image but the number of child flex boxes. It would be nice to fix this at the flex-box level instead of forcing sizing on the image.
To fix this at the flex level you just need to add min-height: 1px
to the .mgtAdvancedSettings__field
and to the EuiFormRow
(will need a custom className as well).
You could also add the size prop to EuiImage
so that it doesn't get to large. I tried it with size="m"
and it looked pretty good.
Oh and since these are IE specific fixes, you could wrap them in the EUI SASS helper like: @include internetExplorerOnly {
min-height: 1px;
} Or at least add a comment to the line that it's fixing IE. |
💔 Build Failed |
@cchaos Ah, I tried that as lined out here philipwalton/flexbugs#75 but I missed the fact I had to apply it to all flex layers, not just the last one. Thanks a lot, will fix it tomorrow. Regarding the image size - wouldn't it still look strange if the user provided an image with a large height and a small width? By setting both max-width and max-height it is made sure that the image is always displayed as a small preview thumb. |
Great thanks!
That might be something we want to address on the EUI side, since, yes it could be odd that we're only maxing the width: But for this particular case, since it's for the footer of a PDF, if they upload something that's very tall it will probably take up more space on the page than they're expecting so I like that it enhances showing the height vs width of the image. |
Opened issue in EUI (elastic/eui#1554) and added
For the moment it's the only image in settings but there might be others in the future so I'm not sure whether we should look too much on the ordinary aspect ratios of this image. Maybe 100px is too little to make something like this stand out, what about 400px? |
💔 Build Failed |
💔 Build Failed |
Jenkins, test this. Flaky test? |
💚 Build Succeeded |
src/legacy/core_plugins/kibana/public/management/sections/settings/advanced_settings.scss
Outdated
Show resolved
Hide resolved
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.
Great, ty!
Retest |
💚 Build Succeeded |
Summary
Fixes #22222
This PR fixes the problem of the above issue by setting a fixed value to the height of the preview image.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately