-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
UI: Fix display of custom brand image #13355
Conversation
👏 Great work, I didn't think it was possible. I added some stories to test the layout with an unsized SVG (i.e. one that doesn't have a width/height prop). Also I reduced some margin so the logo can be a bit bigger. |
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. Do check the Chromatic tests.
Thanks for adding the stories, Gert! Looks like we have one failing story, Standard No Link: https://www.chromatic.com/component?appId=5a375b97f4b14f0020b0cda3&name=UI%2FSidebar%2FHeading&mode=interactive&buildNumber=20261&specName=Standard+No+Link Which seems related to whether or not an anchor is rendered around the image. I suspect a "Custom Brand Image No Link" story would be incorrect, as well. I'll take a look asap. |
Additionally this refactors the Brand component for readability.
@kylegach I fixed the misaligning image and also refactored the Brand component because it was pretty complicated. |
UI: Fix display of custom brand image
Issue: #13313
When supplying
theme.brandImage
, the image renders very small, effectively invisible:What I did
The (Brand) (which renders the selected element in the screenshot, above) is inside a flex container (BrandArea), so I applied
flex: 1 1 auto
(and removed the now redundantwidth: auto
) to BrandArea's immediate descendants to fill the available width. It was previously (implicitly) using the default offlex: 0 1 auto
.How to test
If your answer is yes to any of these, please make sure to include it in your PR.