Add ability to override client assets : logo - favicon - PWA icons - PWA manifest name and description #2897

kimsible · 2020-06-24T15:17:32Z

This PR allows to customize main assets like logo, favicon and PWA icons, PWA manifest name and description :

Add client-overrides directory storage to config files ;
Serve overrides files (logo, favicon, PWA icons) from client-overrides dir with dist dir as fallback ;
Move logo background-image from CSS bundle to <style> tag header in index.html ;
Handle cache for all overrides files with a querystring ?[fileContentHash] ;
Make the PWA manifest a dynamic JSON to update description, name, short_name with instance config ;
~~Add ability in the admin panel to upload a SVG / PNG logo and generate favicon - PWA icons.~~

To test it the client must be built with npm run clean:client and npm run dev:server

kimsible · 2020-06-24T15:20:41Z

@rigelk @Chocobozzz I can also split this PR into several, like 3 more, one for the manifest update, one for the cache, one for the front admin panel.

Do you have an idea where server/controllers/client.ts routes are override in develop mode ?

client/src/app/app.component.html

rigelk · 2020-06-24T18:03:07Z

server/controllers/client.ts

+
+for (const staticClientImage of staticClientImages) {
+  let imagePhysicalPath = join(CONFIG.STORAGE.ASSETS_DIR, 'images', staticClientImage)
+  clientsRouter.use(`/client/assets/images/${staticClientImage}`, asyncMiddleware(async (req: express.Request, res: express.Response, next: express.NextFunction) => {


please split the use and definition of the middleware.

why not use express.static here?

I thought it was better to only serve files we need, do you think we should be able to serve all the assets directory ?

The assets directory only has the files we need to serve, so yes 🙂

It has the advantage to be pretty rich when it comes to built-ins: https://expressjs.com/en/4x/api.html#express.static

The assets directory only has the files we need to serve, so yes

There are all assets of the theme, there is more than we want to serve, no ?

Or you means for the icons ? I think favicon and logo should serve alone.

rigelk · 2020-06-24T18:07:30Z

@kimsible unless you need tests running, please open WIP PRs as WIP.

rigelk · 2020-06-24T18:12:14Z

Add ability in the admin panel to upload a SVG / PNG logo and generate favicon - PWA icons.

A lot of work for something that is trivial to do manually, especially since it is usually done once. I would much rather see a script in the tools, at least to begin with.

kimsible · 2020-06-24T19:05:43Z

Add ability in the admin panel to upload a SVG / PNG logo and generate favicon - PWA icons.

A lot of work for something that is trivial to do manually, especially since it is usually done once. I would much rather see a script in the tools, at least to begin with.

Yes, you're right, it's really asked we should keep it in mind for further PeerTube versions, changing the logo is a common habit when it comes to animate a community for a special event. And for PWAs icons, it's now in the PeerTube Core, generating them is very useful when all the tools are included to do it automatically but in my opinion (I may be wrong), the webmanifest and icons should have been a plugin and not included by default in the client assets build.

The first big issue is the ability to update logo and favicon without doing tricky code with shell or a plugin for each restart or update of PeerTube. Really, there is not so much PeerTube instances with a custom logo for all these reasons, developing a theme is not reachable by anybody.

So, I delete this step =)

Do you think we have to create another PR for the manifest or use this one ?

kimsible · 2020-06-24T19:07:55Z

@kimsible unless you need tests running, please open WIP PRs as WIP.

Ok, you meant as a draft ?

rigelk · 2020-06-24T19:08:16Z

Yes, as draft, sorry ^^'

kimsible · 2020-06-24T19:11:30Z

Yes, as draft, sorry ^^'

Sorry, some things need to be discussed.

Chocobozzz · 2020-06-25T09:18:37Z

Having another storage directory that implicitly override client assets is not something I think I like.

Another proposal:

Remove the manifest file from client and serve the JSON dynamically from the server (so you correctly use the peertube name/description etc from the config, and can add plugins hooks to change the icon paths)
Create a plugin that allows admins to easily upload images from the web interface. The plugin just overrides logos/favicon using hooks and/or css rules

Not sure if it's a better solution than the one implemented in this PR. I prefer the plugin solution, because we don't add implicit things (assets directory), do not add breaking changes (icon displayed using CSS) and use an existing system (plugin API, even if there are some hooks/settings buttons that are still missing). What do you think?

kimsible · 2020-06-25T10:35:59Z

Hi @Chocobozzz , thanks for your participation.

Having another storage directory that implicitly override client assets is not something I think I like.

Create a plugin that allows admins to easily upload images from the web interface. The plugin just overrides logos/favicon using hooks and/or css rules

I proposed to only override logo.svg, favicon.png and PWA icons in the storage assets directory.
It's not all the assets directory, if it was, yes, it could break the main theme and the instance could not benefit of the theme updates. I'm not against from a plugin but it would need some new important features.

Remove the manifest file from client and serve the JSON dynamically from the server (so you correctly use the peertube name/description etc from the config, and can add plugins hooks to change the icon paths)

Serving the JSON manifest dynamically is a very good idea and seems implementable with not so much efforts, but I partially agree for the others assets :

The problem with the plugin is that the system does not allow to simply upload files for the moment, (I actually add some input file in the manifest plugin https://github.com/kimsible/peertube-plugin-webapp-manifest, and it's a real pain to to hook the client, plus sending files via plugins routes is not really secure, the router plugin should be able to upload only in one directory isolated from the app and available only for admins).

What we need new with plugins :

A real upload system with an isolated directory, a route available for admins only, a cache naming, and an upload input file with the preview
Hooks to override the express route of these assets (logo, favicon and PWA icons) and the manifest icons fields
Add the image tool as a helper (Jimp ?)

do not add breaking changes (icon displayed using CSS)

You mean the logo ?
In any case the logo should be displayed as a svg object, it will allow to handle cache naming with query string.

I see another problem with the plugin API, edit the logo and the favicon are really one of the most basic feature we could expect from a self-hosted webapp, the upload input file should be in the admin panel in the config Appearance part.

So, yes this solution is interesting but force us to implement / extend the plugin API support and put a basic feature into a plugin that should be in the PeerTube Core (I mean for the favicon and the logo, I don't think the manifest is a standard outside android ?).

I'm really not against this solution but the plugin API should have all the features I mentioned and the plugin itself should be an official plugin.

What do you think?

kimsible · 2020-06-25T11:22:13Z

@Chocobozzz I have an (intermediate) solution :

Let the admin put logo.svg and favicon.png into the root of storage/ and PWAs icons in storage/pwa ;
Keep the actual router fallback to ./client/dist/assets and make it work for dev mode ;
As @rigelk said, we could create a script in the tools to generate icons and favicon according to ./storage/logo.svg and update cache namnig with querystrings ;
Make the manifest dynamic ;
Add some docs.

EDIT:

config/default.yml will look like :

storage:
  tmp: 'storage/tmp/' # Use to download data (imports etc), store uploaded files before processing...
  ...
  logo: 'storage/logo.svg'
  favicon: 'storage/favicon.png'
  pwa_icons: 'storage/pwa_icons'

Chocobozzz · 2020-06-25T12:29:33Z

Okay then. I prefer your first solution than the intermediate one, but with a more explicit name. Something like:

  cache: 'storage/cache/'
  plugins: 'storage/plugins/'
  # Could contain for example assets/images/favicon.png
  # If the file exists, peertube will serve it
  # If not, peertube will fallback to the default file
  client-overrides: 'storage/client-overrides/'

I think your check is still important, for security reasons.

In any case the logo should be displayed as a svg object, it will allow to handle cache naming with query string.

How do you plan to handle cache naming?

Do you have an idea where server/controllers/client.ts routes are override in develop mode ?

Try to update the proxy file: https://github.com/Chocobozzz/PeerTube/blob/develop/client/proxy.config.json

kimsible · 2020-06-25T12:48:01Z

Thanks client-overrides with comments are more explicit =).
But should we specify which ones are overridable ? (with a link to the documentation for example).

How do you plan to handle cache naming?
A general global hash we can add as a querystring to the image url.
Maybe restarting PeerTube could update this hash ?

EDIT: Later it could be included to the admin panel or in a plugin with the ability to update this global hash without restarting PeerTube

Thanks for the tips too about the proxy !

Chocobozzz · 2020-06-25T12:55:26Z

But should we specify which ones are overridable ? (with a link to the documentation for example).

I think a comment would be enough 👍

A general global hash we can add as a querystring to the image url.

But how do you update the client to inject this querystring/hash to the logo URL? Currently, webpack does the job: updating the filename and the client HTML. If peertube automatically updates the filename on admin upload or peertube restart, we still miss the step where we inject this new logo name in the client no?

kimsible · 2020-06-25T13:06:35Z

But how do you update the client to inject this querystring/hash to the logo URL? Currently, webpack does the job: updating the filename and the client HTML.

Some assets need to be updated during runtime and not buildtime, like our logo or favicon.

If peertube automatically updates the filename on admin upload or peertube restart, we still miss the step where we inject this new logo name in the client no?

I'm not sure I understand well your question, maybe, we could fix a naming convention and extension for the assets to override them? (btw overriding always follows the same naming convention ?)

Naming with a querystring

         <object class="icon icon-logo" type="image/svg+xml" data="/client/assets/images/logo.svg?v=:MY GLOBAL HASH">

EDIT: Ok, I understood the whole point @Chocobozzz , I try to propose something in the CSS

…hash

kimsible · 2020-06-25T20:00:42Z

I didn't find how to cancel the route overriding of webpack dev server with the proxy file.

So the simplest way to test is with the client bundles npm run clean:client and npm run dev:server

Chocobozzz · 2020-07-07T09:19:17Z

@rigelk did you review this PR?

rigelk · 2020-07-07T09:23:18Z

@Chocobozzz no, since I thought you didn't agree with the design.

JohnXLivingston · 2020-08-12T16:01:35Z

Is there any documentation (other than the comment in production.yaml file) for this PR?

Should logo.svg and favicon.png be in /var/www/peertube/storage/client-overrides/ or /var/www/peertube/storage/client-overrides/assets/images/ ?
Can we have other image types? (a .ico for the favicon and a .gif for the logo?)
Should peertube be restarted?

JohnXLivingston · 2020-08-12T16:06:04Z

Should logo.svg and favicon.png be in /var/www/peertube/storage/client-overrides/ or /var/www/peertube/storage/client-overrides/assests/images/ ?

I tried both, and it is not working.

kimsible · 2020-08-12T17:49:21Z

@JohnXLivingston you need to restart PeerTube to update the client cache. This is an experimental feature that will be extend : #2752 (comment)

JohnXLivingston · 2020-08-12T21:17:58Z

Hum, I can't make it work... Don't know why.

kimsible · 2020-11-05T08:19:55Z

@JohnXLivingston do you use nginx ? docker ?

JohnXLivingston · 2020-11-05T11:43:32Z

@JohnXLivingston do you use nginx ? docker ?

nginx

rigelk reviewed Jun 24, 2020

View reviewed changes

rigelk marked this pull request as draft June 24, 2020 18:07

rigelk added Type: Discussion 💭 UI non-trivial UI changes, that might need discussion labels Jun 24, 2020

kimsible added 2 commits June 25, 2020 15:35

Add client-overrides storage to config

f7dcce8

Add static-serve for client overrides

ca8a05d

kimsible force-pushed the feat/custom-assets branch from 5248fe4 to 8176e4a Compare June 25, 2020 13:37

kimsible changed the title ~~Add ability to customize assets : logo - favicon - PWA icons - PWA manifest name and description~~ Add ability to override client assets : logo - favicon - PWA icons - PWA manifest name and description Jun 25, 2020

kimsible added 3 commits June 25, 2020 20:49

Move backgroun-image logo from bundle to css tag for runtime content …

22836b7

…hash

Add dynamic JSON manifest

ee4740d

Add content hash for manifest, favicon and logo

176000e

kimsible force-pushed the feat/custom-assets branch from 48204a2 to 176000e Compare June 25, 2020 19:22

kimsible requested a review from rigelk June 25, 2020 20:00

kimsible marked this pull request as ready for review June 25, 2020 20:09

kimsible mentioned this pull request Jun 25, 2020

Customize appearance (logo, favicon, PWA manifest name and icons) #2752

Open

Chocobozzz merged commit caf2aaf into Chocobozzz:develop Jul 10, 2020

kimsible mentioned this pull request Nov 11, 2020

Client overrides (logo, favicon, PWA icons) does't work behind Nginx #3292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to override client assets : logo - favicon - PWA icons - PWA manifest name and description #2897

Add ability to override client assets : logo - favicon - PWA icons - PWA manifest name and description #2897

kimsible commented Jun 24, 2020 •

edited

Loading

kimsible commented Jun 24, 2020

rigelk Jun 24, 2020

rigelk Jun 24, 2020 •

edited

Loading

kimsible Jun 24, 2020 •

edited

Loading

rigelk Jun 24, 2020 •

edited

Loading

kimsible Jun 24, 2020

rigelk commented Jun 24, 2020 •

edited

Loading

rigelk commented Jun 24, 2020 •

edited

Loading

kimsible commented Jun 24, 2020 •

edited

Loading

kimsible commented Jun 24, 2020

rigelk commented Jun 24, 2020

kimsible commented Jun 24, 2020

Chocobozzz commented Jun 25, 2020

kimsible commented Jun 25, 2020 •

edited

Loading

kimsible commented Jun 25, 2020 •

edited

Loading

Chocobozzz commented Jun 25, 2020 •

edited

Loading

kimsible commented Jun 25, 2020 •

edited

Loading

Chocobozzz commented Jun 25, 2020

kimsible commented Jun 25, 2020 •

edited

Loading

kimsible commented Jun 25, 2020

Chocobozzz commented Jul 7, 2020

rigelk commented Jul 7, 2020 •

edited

Loading

JohnXLivingston commented Aug 12, 2020 •

edited

Loading

JohnXLivingston commented Aug 12, 2020

kimsible commented Aug 12, 2020

JohnXLivingston commented Aug 12, 2020

kimsible commented Nov 5, 2020

JohnXLivingston commented Nov 5, 2020

Add ability to override client assets : logo - favicon - PWA icons - PWA manifest name and description #2897

Add ability to override client assets : logo - favicon - PWA icons - PWA manifest name and description #2897

Conversation

kimsible commented Jun 24, 2020 • edited Loading

kimsible commented Jun 24, 2020

rigelk Jun 24, 2020

Choose a reason for hiding this comment

rigelk Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

kimsible Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

rigelk Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

kimsible Jun 24, 2020

Choose a reason for hiding this comment

rigelk commented Jun 24, 2020 • edited Loading

rigelk commented Jun 24, 2020 • edited Loading

kimsible commented Jun 24, 2020 • edited Loading

kimsible commented Jun 24, 2020

rigelk commented Jun 24, 2020

kimsible commented Jun 24, 2020

Chocobozzz commented Jun 25, 2020

kimsible commented Jun 25, 2020 • edited Loading

kimsible commented Jun 25, 2020 • edited Loading

Chocobozzz commented Jun 25, 2020 • edited Loading

kimsible commented Jun 25, 2020 • edited Loading

Chocobozzz commented Jun 25, 2020

kimsible commented Jun 25, 2020 • edited Loading

kimsible commented Jun 25, 2020

Chocobozzz commented Jul 7, 2020

rigelk commented Jul 7, 2020 • edited Loading

JohnXLivingston commented Aug 12, 2020 • edited Loading

JohnXLivingston commented Aug 12, 2020

kimsible commented Aug 12, 2020

JohnXLivingston commented Aug 12, 2020

kimsible commented Nov 5, 2020

JohnXLivingston commented Nov 5, 2020

kimsible commented Jun 24, 2020 •

edited

Loading

rigelk Jun 24, 2020 •

edited

Loading

kimsible Jun 24, 2020 •

edited

Loading

rigelk Jun 24, 2020 •

edited

Loading

rigelk commented Jun 24, 2020 •

edited

Loading

rigelk commented Jun 24, 2020 •

edited

Loading

kimsible commented Jun 24, 2020 •

edited

Loading

kimsible commented Jun 25, 2020 •

edited

Loading

kimsible commented Jun 25, 2020 •

edited

Loading

Chocobozzz commented Jun 25, 2020 •

edited

Loading

kimsible commented Jun 25, 2020 •

edited

Loading

kimsible commented Jun 25, 2020 •

edited

Loading

rigelk commented Jul 7, 2020 •

edited

Loading

JohnXLivingston commented Aug 12, 2020 •

edited

Loading