-
Notifications
You must be signed in to change notification settings - Fork 355
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
webui: add a logo to the header #5046
webui: add a logo to the header #5046
Conversation
adamkankovsky
commented
Aug 15, 2023
•
edited by KKoukiou
Loading
edited by KKoukiou
df3bbb2
to
0297eb2
Compare
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.
This looks good to me.
My only concern is if this adds vertical space to the header, as the logo is probably taller than the text. If it doesn't, then that's great! If it does, we might need to do something like one of the following:
- logo would either need a vertical margin to compensate, like
margin-block: -0.5rem;
or something like that - the padding for the heading would need to be adjusted
- the position would need to be changed (like absolute with a 2.5rem (size + gap)
- use
background
for sizing
(Using absolute positioning would be the hackiest approach.)
But: Hopefully the logo doesn't affect the size of the heading!
@garrett it does not affect the size of the heading. |
/kickstart-test --waive webui |
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 don't think we should do it like this (shipping a symlink, that will be a dead symlink on boot.iso). Instead I think we should just bundle the icon (until Lorax is fixed not to remove it on boot.iso), which would make this change more robust & simpler code wise.
@@ -64,6 +66,7 @@ export const AnacondaHeader = ({ beta, title, reportLinkURL }) => { | |||
return ( | |||
<PageSection variant={PageSectionVariants.light}> | |||
<Flex spaceItems={{ default: "spaceItemsSm" }} alignItems={{ default: "alignItemsCenter" }}> | |||
{!isBootIso && <span className="logo" />} |
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 should just make sure the icon is available on both boot.iso and Live image, even if we have to bundle it for now.
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.
weldr/lorax#1341 is now merged so in a few days there will no be any broken symlink.
So your concern will be solved.
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.
Giving an official 👍: Looks good to me, based on the screenshot.
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.
Looks good to me.
I prefer this solution more than bundling the icon because if we start bundling it we need to take responsibility to update the icon in case it change. We usually don't know about these changes.
Note, packages aren't allowed to bundle logos. All trademarks have to be put in the package that has Licensed only for approved usage, see COPYING for details. for the license (fedora-logos). |
Just tested - not yet available on boot.iso |
0297eb2
to
adfbc2a
Compare
adfbc2a
to
208c88f
Compare
Co-authored-by: Katerina Koukiou <[email protected]>
208c88f
to
208d125
Compare
/kickstart-test --waive webui |
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.
As apparently the necessary icon is now available also on the boot.iso, I'm fine with the PR in its current form. Thanks! :)