Feat: rebranding, possibility for own brand#24
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 server-side rebranding. The approach of using BRANDING_DIR to overlay custom branding is clean. Here are some observations:
Suggestions:
-
server.bake.Dockerfile—COPY ${BRANDING_DIR}/server/ /server(line 48):
This overwrites the entire/serverdirectory, not just branding-specific files. If a custom branding dir doesn't include all server subdirectories, the build will silently drop missing files. Consider a more targeted approach (e.g.,COPY --no-chown ${BRANDING_DIR}/server/Common/ /server/Common/for specific paths) or add a validation step to ensure all required files are present. -
webpack.config.js— hardcoded fallback (line 11):
const appName = process.env.APP_NAME || 'Euro-Office';hardcodes'Euro-Office'as the fallback. For a rebranding PR, this should be'DocumentServer'or left undefined so upstream defaults apply. Otherwise any build withoutAPP_NAMEexplicitly set will still produce Euro-Office branded output. -
CSS comment stale after color change —
Tabs.module.scssline 45:
/* ensure orange while focused/pressed */is now incorrect since the color was changed to blue. Worth a quick cleanup. -
process.env.APP_NAMEin React components —Menu.js,LoginPage.js,SetupPage.js:
Readingprocess.env.APP_NAMEdirectly in React components works because webpack'sDefinePluginreplaces it at build time, but this is implicit. Consider adding a comment or using a dedicated config module to make this explicit and prevent confusion for future contributors. -
Missing newline at EOF —
server.bake.Dockerfileends without a trailing newline (git diff shows\ No newline at end of file). Minor but worth fixing. -
APP_NAMEvsCOMPANY_NAMEnaming (line 52):
The env var is namedAPP_NAMEbut set toCOMPANY_NAME. For brands where the company name differs from the product/app name (e.g., "Nextcloud" as company but "Nextcloud Office" as app name), this limits flexibility. Consider separatingAPP_NAMEandCOMPANY_NAMEas distinct variables.
Overall: Solid work. Once the webpack fallback and BRANDING_DIR overwrite concerns are addressed, this is ready to merge.
|
webpack fallback is desired behavior. ${BRANDING_DIR} overwrite behavior is ok for now imho. |
No description provided.