Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theming #624

Merged
merged 20 commits into from
Aug 20, 2019
Merged

Theming #624

merged 20 commits into from
Aug 20, 2019

Conversation

elegaanz
Copy link
Member

@elegaanz elegaanz commented Jun 21, 2019

  • 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

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
@elegaanz elegaanz added A: Federation Stuff related to Federation A: Front-End Related to the front-end C: Feature This is a new feature S: Incomplete This PR is not complete yet P: Medium A: Backend Code running on the server labels Jun 21, 2019
@elegaanz elegaanz added this to the 1.0 milestone Jun 21, 2019
elegaanz added 5 commits June 21, 2019 11:03
This way, if all the dark themes contain "dark" in their name, blog themes can detect
dark themes and change accordingly with:

html[class*=dark]

We will need to document that, and the fact that dark theme sould always contain "dark" in their names.
@elegaanz elegaanz added S: Ready for review This PR is ready to be reviewed and removed S: Incomplete This PR is not complete yet labels Jun 21, 2019
@codecov
Copy link

codecov bot commented Jun 21, 2019

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

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trinity-1686a
Copy link
Contributor

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

@elegaanz
Copy link
Member Author

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

- rework assets compilation
- blog themes are now managed at instance level by admins too
- remote blog themes need to be approved by an admin
- once approved, a remote theme is also available on this instance
@elegaanz
Copy link
Member Author

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
Copy link

trwnh commented Jun 28, 2019

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.

elegaanz added 2 commits June 30, 2019 18:03
It makes everythinh easier both for us and for instance admins, and it didn't added a lot of value.

Thank you @trwnh for the suggestion! :)
@trinity-1686a
Copy link
Contributor

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
Copy link
Member Author

elegaanz commented Jul 3, 2019

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

@trinity-1686a
Copy link
Contributor

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
Copy link
Member Author

elegaanz commented Jul 3, 2019

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
Copy link
Contributor

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
Copy link
Member Author

elegaanz commented Jul 3, 2019

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

@elegaanz elegaanz removed the P: Medium label Aug 1, 2019
Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@elegaanz elegaanz merged commit a6c84da into master Aug 20, 2019
@elegaanz elegaanz deleted the themes branch August 20, 2019 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Backend Code running on the server A: Federation Stuff related to Federation A: Front-End Related to the front-end C: Feature This is a new feature S: Ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow customised blogs at user-level
4 participants