Feat: rebrand, possibility for own brand#9
Conversation
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
chrip
left a comment
There was a problem hiding this comment.
Good work on the integration layer rebranding. The appName config approach for the node example is clean, and the consistent color swap across all example apps (csharp, go, java, php, python, ruby) is thorough. Here are some observations:
Suggestions:
-
example.bake.Dockerfile—sedon config JSON (lines 32-33):
sed -i "s/Euro-Office/${COMPANY_NAME}/g"on config files is fragile. If a config value legitimately contains "Euro-Office" as a non-branding string, it will be incorrectly replaced. Also, this only replacesEuro-Office(title case) — the config files I see in the diff use"Euro-Office"as theappNamevalue, but the original diff also had"documentserver"references in other repos. Make sure the case-sensitive match is intentional and consistent with the other repos' branding approach. -
example.bake.Dockerfile—COPY ${BRANDING_DIR}/document-server-integration/web/documentserver-example/nodejs /app(line 30):
This overlays branding on top of the existing/appdirectory, which is the right approach (unlike theserverrepo which overwrites everything). However, if the branding dir doesn't include all files (e.g., missingapp.js), the build will silently use the original files. Consider adding a check that critical branding files exist before the copy. -
mobile-logo.svg→mobile-logo_eo.svgrename —forgotten.ejs,index.ejs,wopiIndex.ejs:
The logo file is renamed tomobile-logo_eo.svgbut the originalmobile-logo.svgis not removed. This leaves a dead file in the repo and could cause confusion. Also, thefavicon_eo.svgreference in the views suggests a favicon rename that doesn't appear in the diff — is that in the branding dir? -
Hardcoded
"Euro-Office"in all config files —development-linux.json,development-mac.json,development-windows.json,production-linux.json,production-windows.json:
All configs now have"appName": "Euro-Office"hardcoded. For a rebranding PR, these should either be"DocumentServer"(upstream default) or removed entirely so the upstream default applies. Otherwise every developer or builder gets "Euro-Office" branding by default. -
editor.ejs— inline<%= appName %>inonMetaChange(line 98):
document.title = event.data.title + " - <%= appName %>";— this EJS template string is being passed to the client-side JS. The<%= appName %>will be rendered server-side during template compilation, so this works, but it's easy to miss that the template is being evaluated twice (once by EJS, once by the browser). Consider documenting this to avoid confusion. -
Color changes across 6 example apps — all
#FF6F3D→#0082C9:
This is a lot of CSS changes across many example apps. While the blue color is consistent, consider whether each example app (csharp, go, java, php, python, ruby) should share a common stylesheet or if maintaining 6 separate CSS files long-term is sustainable.
Overall: Solid rebranding work. Once the hardcoded "Euro-Office" defaults are fixed to upstream values and the logo file situation is cleaned up, this is ready to merge.
No description provided.