Open syeopite opened 3 weeks ago
Related: https://github.com/iv-org/invidious/issues/4113#issuecomment-1732572843
The best way to go about it would be to probably use a CSS pre-processor as then we won't have to use any of the new CSS features and can continue to support old browsers.
I've though about that option, but I'm hesitant to require yet another development tool.
Alternatively if we were to use new CSS features like variables we could implement something akin to this [...]
This is probably the best option imo. We're already planning on dropping IE support with VideoJS 8, so no browser prevents us from using --var
anymore!
I started to fool around with the design with this issue in mind.
In my experiment, I dropped all the CSS and started from scratch. This highlighted some improvements in the HTML to allow better targeting of elements.
Here is my current perspective:
Instead of styling specific classes, using classes that specify different parts of the page. Use raw semantic HTML where possible.
<div>
<div class="pure-x">
<div>
<div>
</div>
</div>
</div>
</div>
to
<main role="main">
<section class="videos">
</section>
</main>
the ecr
templates need to be compiled so the current process involves a reloader
while sleep 0.25; do
tree -fiA --prune --noreport src | entr -rd make run
done
There is a file cache for static file handling. src/ext/kemal_static_file_handler.cr
. I am manually commenting this out to allow me to refresh to get new CSS to load. Ideally we should have this be a configuration option to turn on/off. I think it's usage in production is worthwhile as it currently works.
# if @cached_files.sum(&.[1][:data].bytesize) + (size = File.size(file_path)) < CACHE_LIMIT
# data = Bytes.new(size)
# File.open(file_path, &.read(data))
#
# @cached_files[file_path] = {data: data, filestat: file_info}
# send_file(context, file_path, data, file_info)
# else
send_file(context, file_path)
# end
I appreciate not having a build process for CSS/JS. If desired, a build process could improve feedback of development for styles. Spoiled with hot fast reloading in other projects but more of a side thing here.
Maybe I should had realized the Makefile was there but also needing some Crystal specific things.
And then Makefile will set up the rest of it.
make
:root {
--fg-color: #b1d7d0;
--bg-color: #1a201f;
--accent-fg-color: #b0d5ce;
--accent-bg-color: #02241f;
/* https://github.com/system-fonts/modern-font-stacks */
--sans-serif: ui-rounded, "Hiragino Maru Gothic ProN", Quicksand, Comfortaa,
Manjari, "Arial Rounded MT", "Arial Rounded MT Bold", Calibri,
source-sans-pro, sans-serif, "Apple Color Emoji", "Segoe UI Emoji",
"Segoe UI Symbol";
--monospace: ui-monospace, "Cascadia Code", "Source Code Pro", Menlo, Consolas,
"DejaVu Sans Mono", monospace, "Apple Color Emoji", "Segoe UI Emoji",
"Segoe UI Symbol";
}
body {
color: var(--fg-color);
background-color: var(--bg-color);
font-family: var(--sans-serif);
font-size: 1rem;
line-height: 1.5;
}
Using CSS variables, and native font stacks.
Named CSS selectors for their type.
.comments {} /* list of comments */
.videos {} /* list of videos */
.item {} /* video list block item */
.channel {}
My current desire is to work with the team and make a plan forward, considering these details. I am open to experimentation with changes and improvements. Let me know what you think.
Thank you.
@rockerBOO overall, thanks for your insight and the detailed answer, this is really appreciated :)
Improve semantic details in the HTML [...]
<div class="pure-x">
[...]<section class="videos">
For all the pure-
classes, it's coming from the pure CSS framework. If my memory serves well, it's primarily used for responsiveness and styling some small elements like buttons. I wouldn't mind getting rid of it, but I guess it will depend on how much work this represents.
As for the semantic in HTML, this is tracked in https://github.com/iv-org/invidious/issues/2837.
Side note: the <section>
element can only be used with a section heading (<h2/3/4>
).
the
ecr
templates need to be compiled so the current process involves a reloader
The goal of ECR files is to generate code at compile time. They're not meant to be reloaded dynamically. In https://github.com/iv-org/invidious/issues/4483, syeopite proposed using Jinja templates, which is imo a great option, but has probably the drawback of getting more runtime errors.
There is a file cache for static file handling.
src/ext/kemal_static_file_handler.cr
. I am manually commenting this out to allow me to refresh to get new CSS to load.
Small CSS/JS assets are cached in memory to save invidious from opening a million file descriptors. This has a huge performance impact on large instances! If you want to reload these assests, all you need to do is restart invidious.
Improved documentation for setting up the project
https://docs.invidious.io/installation should have all the basics, but I agree that it's far from perfect. I've also been working on a bunch of contribution guidelines in https://github.com/iv-org/invidious/pull/4591. Don't hesitate to provide feedback there :)
Thank you @SamantazFox ! Thanks for the links to the associated tickets.
As for the semantic in HTML, this is tracked in https://github.com/iv-org/invidious/issues/2837.
Side note: the
element can only be used with a section heading (<h2/3/4>).
Certainly want to use the correct semantic HTML here.
The goal of ECR files is to generate code at compile time. They're not meant to be reloaded dynamically. In https://github.com/iv-org/invidious/issues/4483, syeopite proposed using Jinja templates, which is imo a great option, but has probably the drawback of getting more runtime errors.
ECR mentioned was more of a note that it makes some of the process slower but can be worked around. It's probably a bigger process than making this more theme-able. Certainly appreciate the runtime error protection.
Small CSS/JS assets are cached in memory to save invidious from opening a million file descriptors. This has a huge performance impact on large instances! If you want to reload these assets, all you need to do is restart invidious.
Static file handler I mentioned because it caches even when its local in development. That makes it impossible to reload a CSS file without restarting the invidious process. So having it be a setting that can be set to off during development would allow the static files to not need the caching.
https://docs.invidious.io/installation should have all the basics, but I agree that it's far from perfect. I've also been working on a bunch of contribution guidelines in https://github.com/iv-org/invidious/pull/4591. Don't hesitate to provide feedback there :)
Ahh, this is good. I will give feedback.
My thoughts right now is it might be a good iterative process. What is the quickest, forward-thinking approach to allowing better theming.
My opinion is:
Use CSS variables as they are widely used and supported at 97.75% global usage. Key missing support in Opera Mini and IE 11.
Update the HTML to use valid semantic HTML, and add in additional classes to help with selectors.
These could be done iteratively, and have a low impact on current users with custom designs. But if we change the semantics, the selectors may be different if they target <div>
over another tag. Especially likely due to a lack of selector available classes.
Let me know if this is a good approach to start experimenting with.
Thank you.
Is your enhancement request related to a problem? Please describe.
With the way theming is currently done in the CSS style logic has to be duplicated around four times in different parts of the stylesheet.
This is a maintaince and development nightmare.
Describe the solution you'd like
A better way of theming that doesn't require such a duplication.
The best way to go about it would be to probably use a CSS pre-processor as then we won't have to use any of the new CSS features and can continue to support old browsers.
Describe alternatives you've considered
Alternatively if we were to use new CSS features like variables we could implement something akin to this
https://css-tricks.com/a-dry-approach-to-color-themes-in-css/
Though this solution breaks the darkreader extension
Additional context