Theming #624

Слито
elegaanz слито 20 коммит(ов) из themes в master 5 лет назад
elegaanz прокомментировал(а) 5 лет назад (Перенесено из github.com)
  • Custom CSS for blogs
  • Custom themes for instance
  • New dark theme
  • UI for admin to upload new theme (or delete them)
  • UI to choose your theme

image

Instance level themes are just CSS files, stored in static/css (see Instance::list_themes).

TODO:

  • Federate blog themes
  • Escape url()/import
  • Add a way to avoid conflicts between blog and instance themes
  • UI to disable blog CSS (the option is already in the database, just need to add a checkbox in the settings)
  • SQlite migrations
  • Documentation

Fixes #354, #403

- Custom CSS for blogs - Custom themes for instance - New dark theme - UI for admin to upload new theme (or delete them) - UI to choose your theme ![image](https://user-images.githubusercontent.com/16254623/59910472-de7f1e00-9409-11e9-9d24-6e369f960608.png) Instance level themes are just CSS files, stored in `static/css` (see `Instance::list_themes`). TODO: - [x] Federate blog themes - [x] Escape url()/import - [x] Add a way to avoid conflicts between blog and instance themes - [x] UI to disable blog CSS (the option is already in the database, just need to add a checkbox in the settings) - [x] SQlite migrations - [x] Documentation Fixes #354, #403
codecov[bot] прокомментировал(а) 5 лет назад (Перенесено из github.com)

Codecov Report

Merging #624 into master will decrease coverage by 0.32%.
The diff coverage is 12%.

@@           Coverage Diff            @@
##           master   #624      +/-   ##
========================================
- Coverage   35.33%    35%   -0.33%     
========================================
  Files          68     68              
  Lines        7915   7938      +23     
  Branches     1894   1888       -6     
========================================
- Hits         2797   2779      -18     
- Misses       4341   4381      +40     
- Partials      777    778       +1
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/624?src=pr&el=h1) Report > Merging [#624](https://codecov.io/gh/Plume-org/Plume/pull/624?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/fb60236a54893d690a8ddba34f597e4820431154?src=pr&el=desc) will **decrease** coverage by `0.32%`. > The diff coverage is `12%`. ```diff @@ Coverage Diff @@ ## master #624 +/- ## ======================================== - Coverage 35.33% 35% -0.33% ======================================== Files 68 68 Lines 7915 7938 +23 Branches 1894 1888 -6 ======================================== - Hits 2797 2779 -18 - Misses 4341 4381 +40 - Partials 777 778 +1 ```
igalic (Перенесено из github.com) рассмотрел(а) изменения 5 лет назад
igalic (Перенесено из github.com) оставил комментарий

@ -0,0 +1,4 @@
-- Your SQL goes here
ALTER TABLE blogs ADD COLUMN theme VARCHAR;
ALTER TABLE users ADD COLUMN preferred_theme VARCHAR;
igalic (Перенесено из github.com) прокомментировал(а) 5 лет назад

this should be a cookie setting

I really hope this is a cookie setting.

this should be a cookie setting I really hope this is a cookie setting.
elegaanz (Перенесено из github.com) рассмотрел(а) изменения 5 лет назад
@ -0,0 +1,4 @@
-- Your SQL goes here
ALTER TABLE blogs ADD COLUMN theme VARCHAR;
ALTER TABLE users ADD COLUMN preferred_theme VARCHAR;
elegaanz (Перенесено из github.com) прокомментировал(а) 5 лет назад

Why should it be a cookie setting? Storing it in the database as the advantage of having it synced across devices (and also it will never expire).

Why should it be a cookie setting? Storing it in the database as the advantage of having it synced across devices (and also it will never expire).
igalic (Перенесено из github.com) рассмотрел(а) изменения 5 лет назад
@ -0,0 +1,4 @@
-- Your SQL goes here
ALTER TABLE blogs ADD COLUMN theme VARCHAR;
ALTER TABLE users ADD COLUMN preferred_theme VARCHAR;
igalic (Перенесено из github.com) прокомментировал(а) 5 лет назад

you'd have to sync it to the cookie, but that way it'd be easier & quicker to access.

you'd have to sync it to the cookie, but that way it'd be easier & quicker to access.
elegaanz (Перенесено из github.com) рассмотрел(а) изменения 5 лет назад
@ -0,0 +1,4 @@
-- Your SQL goes here
ALTER TABLE blogs ADD COLUMN theme VARCHAR;
ALTER TABLE users ADD COLUMN preferred_theme VARCHAR;
elegaanz (Перенесено из github.com) прокомментировал(а) 5 лет назад

The whole user struct is already loaded for every page anyway, I'm not sure it is going to make a real difference 🤷‍♀️

The whole user struct is already loaded for every page anyway, I'm not sure it is going to make a real difference :woman_shrugging:
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

is it still ready for review considering the design flaw pointed regarding arbitrary css security?

is it still ready for review considering the design flaw pointed regarding arbitrary css security?
elegaanz прокомментировал(а) 5 лет назад (Перенесено из github.com)

No, I'm implementing another approach, I think I will be able to push it this evening.

No, I'm implementing another approach, I think I will be able to push it this evening.
elegaanz прокомментировал(а) 5 лет назад (Перенесено из github.com)

If you want to review this PR, the new version is there. Admins should now approve remote blog themes, but they may contain anything. Once a theme is approved, it is also available for use on this instance. Admins can also upload themes if they have SSH acces to their instance: they just need to create a folder called "blog-SOMETHING" in static/css, and add the theme files inside.

I will update the docs too.

If you want to review this PR, the new version is there. Admins should now approve remote blog themes, but they may contain anything. Once a theme is approved, it is also available for use on this instance. Admins can also upload themes if they have SSH acces to their instance: they just need to create a folder called "blog-SOMETHING" in `static/css`, and add the theme files inside. I will update the docs too.
trwnh прокомментировал(а) 5 лет назад (Перенесено из github.com)

Does it really make sense to have admins approve remote blog themes? Using remote resources on the local site seems fragile and prone to many more errors in the future. I think it makes more sense to limit themes to site-wide and local-user.

More generally, this is an issue that has to do with the way articles federate in Plume -- the HTML is delivered and cached to many different sites that may have varying CSS. Federating CSS as well is, I think, out-of-scope of ActivityPub (although there is nothing preventing this). In any case, it is much simpler to say that users should view the original URL if they wish to read the article with the original CSS.

Does it really make sense to have admins approve remote blog themes? Using remote resources on the local site seems fragile and prone to many more errors in the future. I think it makes more sense to limit themes to site-wide and local-user. More generally, this is an issue that has to do with the way articles federate in Plume -- the HTML is delivered and cached to many different sites that may have varying CSS. Federating CSS as well is, I think, out-of-scope of ActivityPub (although there is nothing preventing this). In any case, it is much simpler to say that users should view the original URL if they wish to read the article with the original CSS.
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

I'm not sure this is an actual concern but how does it play with caching? Where are the file stored and are they given a unique name (hash based?)?

I'm not sure this is an actual concern but how does it play with caching? Where are the file stored and are they given a unique name (hash based?)?
elegaanz прокомментировал(а) 5 лет назад (Перенесено из github.com)

The themes are stored in static/css so they should integrate with the current caching system I think?

The themes are stored in `static/css` so they should integrate with the current caching system I think?
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

I think they are cached, but not evicted when modified, so client might think the file was not updated when it was. Adding a timestamp, a hash, a random string or anything to the name (myfile-DEADBEEF1312.css) would fix it

I think they are cached, but not evicted when modified, so client might think the file was not updated when it was. Adding a timestamp, a hash, a random string or anything to the name (`myfile-DEADBEEF1312.css`) would fix it
elegaanz прокомментировал(а) 5 лет назад (Перенесено из github.com)

I think it is going to be quite difficult, because in the current implementation, the main theme file should be named theme.css, and it may load other files of the theme, so we would have to find a way to detect the main theme file even if it has a hash in its name, and to edit all url()/@import inside of it to make sure other files are correctly loaded too… Maybe we can just set a default cache duration of one week or something like that, with a HTTP header? Or is it a problem not to have the latest theme update for a few days?

I think it is going to be quite difficult, because in the current implementation, the main theme file should be named `theme.css`, and it may load other files of the theme, so we would have to find a way to detect the main theme file even if it has a hash in its name, and to edit all `url()`/`@import` inside of it to make sure other files are correctly loaded too… Maybe we can just set a default cache duration of one week or something like that, with a HTTP header? Or is it a problem not to have the latest theme update for a few days?
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

Even 7 days is quiet long and will definitely be reported as a bug at some point. I'm at work currently, maybe we can discuss of it later on riot?

Even 7 days is quiet long and will definitely be reported as a bug at some point. I'm at work currently, maybe we can discuss of it later on riot?
elegaanz прокомментировал(а) 5 лет назад (Перенесено из github.com)

OK, feel free to ping me when you want to talk about it :)

OK, feel free to ping me when you want to talk about it :)
trinity-1686a рассмотрел(а) изменения 5 лет назад
trinity-1686a оставил комментарий
Владелец

👀

👀
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

????

????
@ -0,0 +83,4 @@
last_fetched_date,
fqn,
summary_html
FROM users;
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

sqlite migration mi amor

sqlite migration mi amor
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

in case of error, return empty Vec? I think returning a Result<...> would make more sens, to have a proper error in case of.. error

in case of error, return empty `Vec`? I think returning a `Result<...>` would make more sens, to have a proper error in case of.. error
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

Same goes

Same goes
trinity-1686a прокомментировал(а) 5 лет назад
Владелец

so this whole function was useless? Some other must be too then

so this whole function was useless? Some other must be too then
elegaanz (Перенесено из github.com) рассмотрел(а) изменения 5 лет назад
elegaanz (Перенесено из github.com) прокомментировал(а) 5 лет назад

Yes, I would like to do a big code cleanup one day, removing all pub, progressively reintroducing them until it compiles, and remove everything that Rust says is unused.

Yes, I would like to do a big code cleanup one day, removing all `pub`, progressively reintroducing them until it compiles, and remove everything that Rust says is unused.
trinity-1686a одобрил(а) эти изменения 5 лет назад
trinity-1686a оставил комментарий
Владелец

lgtm

lgtm

Рецензенты

trinity-1686a одобрил(а) эти изменения 5 лет назад
Запрос на слияние был объединен как a6c84daa1a.
Вы также можете просмотреть инструкции командной строки.

Шаг 1:

В репозитории вашего проекта посмотрите новую ветку и протестируйте изменения.
git checkout -b themes master
git pull origin themes

Шаг 2:

Объединить изменения и обновить на Forgejo.
git checkout master
git merge --no-ff themes
git push origin master
Войдите, чтобы присоединиться к обсуждению.
Нет рецензентов
Нет этапа
Нет назначенных лиц
2 участников
Уведомления
Срок выполнения
Срок действия недействителен или находится за пределами допустимого диапазона. Пожалуйста, используйте формат 'гггг-мм-дд'.

Срок выполнения не установлен.

Зависимости

Зависимостей нет.

Reference: Plume/Plume#624
Загрузка…
Пока нет содержимого.