-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow users the ability to resize the logo #15093
base: master
Are you sure you want to change the base?
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
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.
Nice work @mikesealey!
Before anything else, the original issue here is that the user has a large amount of padding around their logo. Fixing this would make the logo appear much bigger and fix their issue, but moving on...
There are a few usability issues I think we need to address here - the biggest one being that this will significantly reduce the size of all existing logos, as it implies a new max-width constraint.
Today we only enforce a max height of 36px to ensure sensible layout, and let width scale dynamically. If we use the Ford logo as an example, this is how it looks today:
And this is how it looks on this PR:
Increasing the new size setting to something like 100px works to fix it, but by default many apps will be broken by the change as it requires manual intervention, and we shouldn't change any existing apps by default unless we really have to.
Reading the original issue, I think the only change we need is a small one to allow the height to be slightly larger. I think the best way to achieve this would be a logo size setting like you've added, but with options for small, medium (default, 36px as it is today) and large. I don't think we need granular control over the pixel size. I would continue to only use this dimension for height rather than width, as we need to enforce height in order to maintain a sensible layout. I don't think tall aspect ratio logos make sense, and we don't need to worry about support for them.
The addition of the "text below logo" setting makes sense to fix the issue of the text not fitting, but we could make that even better by just wrapping automatically when the text does not fi (when using a side nav). I don't think it needs to be a setting. This would require a few tweaks to the CSS to set up wrapping, so feel free to even leave that out of this PR if that proves tricky.
Description
This PR allows users to resize the logo in the navigation bar.
Addresses
Screenshots
In the logo section of the Navigation settings, we can now set the size (in pixels) by free-typing in the field. There are also a handful of preset options (18, 24, 36, 48, 72, 96) in the dropdown list. Is setting is also bindable, to allow users to set different logo sizes according to the device
When the navigation is set to display vertically on the left of the page, the user will now see a checkbox to display the title below the logo, giving them more space to display larger logos.
There's some work done around sanitising the input from the user in order to only accept a number that is appended with the "px" units. Bindings and JS will work, but if they return something that does not equate to a number, the default 36px sizing will be applied
165px is the largest size that a vertical navigation bar can display without disrupting the layout, so there's a provision that limits valid number-inputs that equate to higher than 165
This provision also accounts for irregularly shaped source images in logos that are either very tall and narrow or very short and wide.
Launchcontrol
Allow users to resize the logo in the navigation