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

[Enhancement] CSS: Overhaul the way theming is done #5056

Open
syeopite opened this issue Nov 7, 2024 · 13 comments
Open

[Enhancement] CSS: Overhaul the way theming is done #5056

syeopite opened this issue Nov 7, 2024 · 13 comments
Labels
enhancement Improvement of an existing feature type:client-side The user interface of invidious

Comments

@syeopite
Copy link
Member

syeopite commented Nov 7, 2024

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

@syeopite syeopite added enhancement Improvement of an existing feature type:client-side The user interface of invidious labels Nov 7, 2024
@SamantazFox
Copy link
Member

Related: #4113 (comment)

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!

@rockerBOO
Copy link

I started to fool around with the design with this issue in mind.
Screenshot 2024-11-15 at 11-22-31 Trending - Invidious

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:

Improve semantic details in the HTML

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>

Improve developer experience

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

Improved iteration speed for CSS/JS

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.

Improved documentation for setting up the project

Maybe I should had realized the Makefile was there but also needing some Crystal specific things.

  • Crystal binary
  • Shard binary

And then Makefile will set up the rest of it.

make

Example CSS

: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 {} 

Future

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.

@SamantazFox
Copy link
Member

@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 #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 #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 #4591. Don't hesitate to provide feedback there :)

@rockerBOO
Copy link

rockerBOO commented Nov 18, 2024

Thank you @SamantazFox ! Thanks for the links to the associated tickets.

As for the semantic in HTML, this is tracked in #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 #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 #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.

  • Use the CSS variables approach?
  • Update the HTML to use semantic HTML, leaving the pure
  • Update the HTML to use semantic HTML,, remove pure, and update the CSS

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.

@rockerBOO
Copy link

rockerBOO commented Nov 30, 2024

Had some time to poke at this. Swapped out the colors and moved most of the positioning values to em/rem. Tried to keep it looking relatively the same as before.

  • Kept all the pure and made some changes from <p> to <div> or <h3> <h4> where appropriate.
  • Updated to the system fonts stack (OS native fonts). Fonts were being set to sans-serif by the pure framework and the body value was not being used.
  • Removed some style="" attributes so it will be style-able.
  • Made a stylelint config to help in updating the code. For my personal approach but I could make it available too.

Screenshot 2024-11-29 at 22-06-48 Invidious
Screenshot 2024-11-29 at 22-06-41 Invidious

:root {
  --sans-serif: ui-rounded, "Hiragino Maru Gothic ProN", "Quicksand",
    "Comfortaa", "Manjari", "Arial Rounded MT", "Arial Rounded MT Bold",
    "Calibri", source-sans-pro, sans-serif;
  --monospace: font-family: ui-monospace, "Cascadia Code", "Source Code Pro",
    "Menlo", "Consolas", "DejaVu Sans Mono", monospace;
}

html,
body {
  font-family: var(--sans-serif);
  background-color: var(--bg-color);
  color: var(--fg-color);
}

.dark-theme {
  --fg-color: #f0f0f0;
  --bg-color: #0d0d0d;
  --accent-color: #c6c6c6;
  --accent-bg-color: #0197ff;
  --secondary-color: #b3b3b3;
  --secondary-bg-color: #0d0d0d;
}

.light-theme {
  --fg-color: black;
  --bg-color: #eee;
  --accent-color: #3a3a3a;
  --accent-bg-color: #008bec;
  --secondary-color: #7c7c7c;
  --secondary-bg-color: #eee;
}

@media (prefers-color-scheme: light) {
  .no-theme {
    --fg-color: black;
    --bg-color: #eee;
    --accent-color: #3a3a3a;
    --accent-bg-color: #008bec;
    --secondary-color: #7c7c7c;
    --secondary-bg-color: #eee;
  }
}

@media (prefers-color-scheme: dark) {
  .no-theme {
    --fg-color: #f0f0f0;
    --bg-color: #0d0d0d;
    --accent-color: #c6c6c6;
    --accent-bg-color: #001f31;
    --secondary-color: #b3b3b3;
    --secondary-bg-color: #0d0d0d;
  }
}

Still needs some color massaging but seems to be OK for the most part.

Let me know if there are any questions or feedback on this approach.

@syeopite
Copy link
Member Author

My only concern with using CSS variables directly is that we'll still have to duplicate the definations at least twice for each theme

@rockerBOO
Copy link

rockerBOO commented Nov 30, 2024

For the duplicates we could do this approach.

  • Default to dark colors
  • Alternate light colors
  • Prefers light (default to light)
  • Selects light-theme
  • Selects dark-theme
:root {
  --fg-color-dark: #f0f0f0;
  --bg-color-dark: #0d0d0d;
  --accent-color-dark: #c6c6c6;
  --accent-bg-color-dark: #0197ff;
  --secondary-color-dark: #b3b3b3;
  --secondary-bg-color-dark: #0d0d0d;

  /* light theme colors */
  --fg-color-light: black;
  --bg-color-light: #eee;
  --accent-color-light: #3a3a3a;
  --accent-bg-color-light: #008bec;
  --secondary-color-light: #7c7c7c;
  --secondary-bg-color-light: #eee;

  --fg-color: var(--fg-color-dark);
  --bg-color: var(--bg-color-dark);
  --accent-color: var(--accent-color-dark);
  --accent-bg-color: var(--accent-bg-color-dark);
  --secondary-color: var(--secondary-color-dark);
  --secondary-bg-color: var(--secondary-bg-color-dark);
}

@media (prefers-color-scheme: light) {
  :root {
    --fg-color: var(--fg-color-light);
    --bg-color: var(--bg-color-light);
    --accent-color: var(--accent-color-light);
    --accent-bg-color: var(--accent-bg-color-light);
    --secondary-color: var(--secondary-color-light);
    --secondary-bg-color: var(--secondary-bg-color-light);
  }
}

.light-theme {
  --fg-color: var(--fg-color-light);
  --bg-color: var(--bg-color-light);
  --accent-color: var(--accent-color-light);
  --accent-bg-color: var(--accent-bg-color-light);
  --secondary-color: var(--secondary-color-light);
  --secondary-bg-color: var(--secondary-bg-color-light);
}

.dark-theme {
  --fg-color: var(--fg-color-dark);
  --bg-color: var(--bg-color-dark);
  --accent-color: var(--accent-color-dark);
  --accent-bg-color: var(--accent-bg-color-dark);
  --secondary-color: var(--secondary-color-dark);
  --secondary-bg-color: var(--secondary-bg-color-dark);
}

@syeopite
Copy link
Member Author

syeopite commented Dec 1, 2024

Isn't that still the same problem? Sure the colors are now defined only once but the usage of those colors for each theme still needs to be applied twice.

This is the only truly DRY approach I've seen to theming in pure CSS

https://css-tricks.com/a-dry-approach-to-color-themes-in-css

But again this does break the Dark Reader extension... I'm not sure about the overlap of users of Invidious and Dark Reader but I am hesitant on introducing CSS "hacks" that'll make it harder for users to use user themes

@rockerBOO
Copy link

In that article it might produce a more DRY result but they also mention it as hacky and not clear. I think the tradeoffs here for clarity might be more important as it lowers complexity for individuals creating modifications to the themes. These showcase the colors but we really only use 3 colors. The additional colors allow for more expression but I do not think we will need significantly more colors. The other variables do not have as much complexity, like fonts, spacing.

Ultimately I am fine with whichever solution is decided.

I use the Dark Reader extension and my current styling improvements work fine with it currently.

Example of how it looks right now.

Screenshot 2024-12-07 at 14-44-45 Trending - Invidious

@rockerBOO
Copy link

rockerBOO commented Dec 12, 2024

Screenshot 2024-12-12 at 14-07-22 NBA - Invidious

Screenshot 2024-12-12 at 14-19-11 NBA - Invidious

Current layout for the channel page.

Screenshot 2024-12-12 at 15-33-54 Are We Witnessing The Greatest Peak In NBA History - Invidious

Screenshot 2024-12-12 at 15-35-19 Are We Witnessing The Greatest Peak In NBA History - Invidious

Comments and replies layout. Trying putting the meta info on the right side but there are some issues with how that looks. I think I might move the time next to the user's name and put the YT and likes/creator like below again.

I'm still chipping away at issues I find. I'm using it mostly all day so iterating on that.

I want to get a version completed here soon to review. May be a multi-step process as the number of changes might be a challenge to review as a whole.

Last bits are

  • Cleaning up the responsive bits
  • Do a few accessibility passes
  • Thin/low profile mode pass
  • Do some HTML layout/naming passes to clean up any lose ends
  • Continue to improve the colors, especially on light theme
  • Language organization/direction tests

Then I would want to go through how best to get these changes reviewed in terms of scope and what works best for the team.

  • Review theming in general (improving variables, CSS colors, spacing and such)
  • Review HTML changes in the templates
  • Review option to disable static file cache for local development (though I found another workaround by putting a reverse proxy in front to serve the static files)

I have some ideas for implementing some popular color schemes as themes as well. Implementing these will probably shed further light on what might be good to improve before it's released.

Open to further feedback on this approach and any input on my reviewing process presented. Thanks

@syeopite
Copy link
Member Author

If you're willing to dig through legacy code you might be able to find some helpful CSS in this behemoth of a PR #2215

@syeopite
Copy link
Member Author

Anyhow for your next update please open up a new issue to continuing discussing this topic.

This issue is for overhauling the way color themes are done specifically and nothing else

It is getting quite off topic here 😅

@rockerBOO
Copy link

Apologies about getting off topic, didn't know the right flow.

Thanks for the PR link, there are some things I can use from that.

Made a design system HTML page to showcase all the elements for theming. I'm still working on various smaller issues but some examples. I think the themes can have full CSS but the following CSS would be a minimal example. Each theme could set dark and/or light. If not set the default version of dark/light will be used.

/assets/css/theme-x.css

:root {
  --fg-color-dark: #f8f8f2;
  --bg-color-dark: #282a36;
  --accent-color-dark: #fff;
  --accent-bg-color-dark: #44475a;
  --secondary-color-dark: #e3e3e3;
  --secondary-bg-color-dark: #21222C;

  /* light theme colors */
  --fg-color-light: black;
  --bg-color-light: #eee;
  --accent-color-light: #3a3a3a;
  --accent-bg-color-light: #008bec;
  --secondary-color-light: #424242;
  --secondary-bg-color-light: #d9d9d9;
}

Some examples of some "themes" but I only really quickly put some colors in from some popular themes.

Screenshot 2024-12-16 at 20-59-08
Screenshot 2024-12-16 at 20-59-44
Screenshot 2024-12-16 at 21-08-33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of an existing feature type:client-side The user interface of invidious
Projects
None yet
Development

No branches or pull requests

3 participants