Added ability to customize logo and title #9052
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #9052 +/- ##
===========================================
+ Coverage 73.42% 73.72% +0.30%
===========================================
Files 419 428 +9
Lines 44351 44507 +156
Branches 3875 3875
===========================================
+ Hits 32563 32812 +249
+ Misses 11788 11695 -93
|
cvat/apps/engine/views.py
Outdated
| @@ -189,6 +189,12 @@ | |||
| _DATA_UPDATED_DATE_HEADER_NAME = 'X-Updated-Date' | |||
| _RETRY_AFTER_TIMEOUT = 10 | |||
|
|
|||
| ICON_FILE = "assets/logo.svg" | |||
|
|
|||
| def get_logo() -> str: | |||
There was a problem hiding this comment.
Please check that we don't read it on every about request.
There was a problem hiding this comment.
Changed implementation to use static logo file as Roman suggested. Now it should be cached correctly
|
|
||
| return ( | ||
| <div className='cvat-logo-icon'> | ||
| <img |
There was a problem hiding this comment.
What if the server returns logo (let's say 1024x256px)? Will it be fit into the component?
There was a problem hiding this comment.
Changed styles a bit. By default logo component is 64x32 so everything bigger will be scaled using object-fit: contain to maintain the size
cvat/apps/engine/views.py
Outdated
| "annotate million of objects with different properties. Many UI " + | ||
| "and UX decisions are based on feedbacks from professional data " + | ||
| "annotation team." | ||
| "logo": get_logo(), |
There was a problem hiding this comment.
Do we really need to include the whole logo into the API response?
I think it would be more sensible to include the URL of the logo, and stick the actual logo in the static directory. Then:
- The API response won't be as bloated.
- The usual HTTP caching mechanisms will apply, so the clients won't fetch the logo more often than necessary.
- It'll be more efficient if the logo is in a non-text format (e.g. PNG), since no encoding will be necessary.
There was a problem hiding this comment.
Makes sense, implemented this approach
That seems normal to me - where else would it be? I'm assuming that the customization process will involve mounting an alternate icon file into the container. Did you have something else in mind? |
| "name": "Computer Vision Annotation Tool", | ||
| "name": settings.ABOUT_INFO["name"], | ||
| "title": settings.ABOUT_INFO["title"], | ||
| "description": settings.ABOUT_INFO["description"], |
There was a problem hiding this comment.
Why did you make the name and description customizable? I don't think that was in the customer's request. It's also not mentioned in the changelog.
There was a problem hiding this comment.
For me it makes sense if we want to customize the /about response via settings we can also customize those fields.
|
|
From my perspective, the logo customization process should be straightforward: place the new file in correct folder and configure a setting for its name if necessary. The same goes for text info in |
| .cvat-signing-header { | ||
| background: transparent; | ||
| position: fixed; | ||
| padding: $grid-unit-size * 2 0 0 0; | ||
| width: 100%; | ||
|
|
||
| .cvat-logo-icon { | ||
| fill: white; | ||
| filter: brightness(0) invert(1); |
There was a problem hiding this comment.
As I understand it makes any logo on sign-in page white, however what if a color logo provided?
This approach will break such logo.
Motivation and context
This simple PR moves logo management to
/aboutserver endpoint. It would make easier to change it if needed by replacinglogo.svgasset on the server. Additinally, there is a possibility to edit title and description using settingsHow has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)License
Feel free to contact the maintainers if that's a concern.