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

add nord #151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add nord #151

wants to merge 1 commit into from

Conversation

b-
Copy link

@b- b- commented Feb 3, 2023

This PR adds the modified theme from #143. I rebuilt the CSS against the latest SASS template from master before committing it.

(@didli didn't have this in a fork on their own GitHub, so I made a branch in mine).

Please don't merge this to master, but rather to a separate branch. That way one can choose to apply Nord (or another theme) by specifying it to the install script. (@Weilbyte you mention in #143 that it would be nice to have a more centralized repo for multiple themes, and I agree. I think multiple branches is one easy way?)

A couple of issues that remain:

  • This reverts the gray/dimmed icons (that were originally introduced in d63f334. I didn't look into why, but I'm guessing it would be an easy fix?
  • Legibility isn't super great
  • I couldn't install my fork by simply running REPO=b-/PVEDiscordDark TAG=nord sudo ./PVEDiscordDark.sh install. It might not be something wrong with the installer — Maybe it's something silly with my shell. Or worse, maybe I'm getting old and forgetting syntax. But either way, there should be a more friendly way to pick a theme if there is more than one choice :)

This update adds the modified theme from Weilbyte#143 as a branch to the main repository

Co-authored-by: didli <[email protected]>
@didli
Copy link

didli commented Feb 3, 2023

Hi @b-

"This reverts the gray/dimmed icons"

Do you mean these icons ?
grey_dimmed
I've updated my theme to look like in this picture but I can't remember if I've uploaded it - -'
(but well, it's still not doing great for legibility)

@b-
Copy link
Author

b- commented Feb 3, 2023

Hi @b-

"This reverts the gray/dimmed icons"

Do you mean these icons ? grey_dimmed I've updated my theme to look like in this picture but I can't remember if I've uploaded it - -' (but well, it's still not doing great for legibility)

It doesn't look like that on mine, no:
image

I added you as a collaborator to my repository if you want to edit the branch that's attached to this PR directly (git clone [email protected]:b-/PVEThemes.git -b nord)

@b-
Copy link
Author

b- commented Feb 3, 2023

By the way, I think it's pretty alright for legibility! Maybe not quite as much so as the stock or upstream PVEDiscordDark themes, but it's a great start!

I think this variant is really awesome, but even more importantly I like the idea of lowering the barrier to making more custom themes — I figure if most custom themes are just recolors of this one (perhaps also swapping images) it shouldn't be too difficult to create some simple tooling for adding more theme variants.

I'm also really not married to the idea of different themes living in different branches. It's probably ideal for development, but it would probably be easier for end users to customize things and create their own variants if it's all in one tree now that I think of it. Forgive me for being unclear — I'm not sure how to really explain this — but maybe we could replace _vars.sass with a directory of themes/*.sass e.g., themes/DiscordDark.sass and themes/nord.sass and then generate relevant full theme directory trees with a script that loops through them.

Relatedly, I'm thinking it might make sense to modify the installer to work slightly differently with regards to offline installation. Presently, the installer checks for the presence of a directory $PWD/offline that is then treated as the cloned repo. I think it would make more sense to check for the presence of e.g., the meta folder, and if it's present then treat $PWD as the cloned repo. And also to set the installer script as mode executable. This would allow for the following offline installation workflow:

$ git clone https://github.com/Weilbyte/PVEDiscordDark
$ cd PVEDiscordDark
$ sudo ./PVEDiscordDark.sh install

We could also add a requirements.txt for pip and allow a sourceinstall method (name subject to change) that would first compile the sass and then do an offline install of it. This could streamline development and setting up a dev environment.

I should put the above into their own issues and PRs, and I probably will at some point, but I'm tired and feeling a little lazy right now so I think I'm going to stop typing and hit the "Comment" button instead. I want to be clear, though, that when I say "we," I mean "anyone interested in working on this" since it's an open source project on Github. I probably will do some of this when I feel up to it.

@didli
Copy link

didli commented Feb 3, 2023

First, thank you for your involvement ! These are some neat ideas that I would like to see. Maybe some will end up in the official proxmox dev git one day (here)
I'm still looking of doing something like this for my own use (but lacks the time) :

  • the installer notices a "theme" folder
  • the installer asks the user to choose (or not) between themes (subfolders inside the parent theme folder)
  • the installer takes the answer as the cloned repo.

Since I don't have much knowledge about github (my own repo is something I did on a whim and forgot all about it until now), I can't clone your branch right now since I don´t know how to get allowed to do so.
So here's a little package until I look into it :
offline-20230203.tar.gz
Let me know if this works for you.

@b-
Copy link
Author

b- commented Feb 3, 2023

Let me know if this works for you

It doesn't really, because it looks to me like you edited the generated CSS and not the files which are used to generate the CSS? That makes it a lot harder to update, or to merge into the repo :(

Let me see what I can do to make it easier to get a working dev environment for this.

@b-
Copy link
Author

b- commented Feb 3, 2023

@didli Here, follow these steps, and then do not edit the css files directly. Instead, edit the sass files, and run the command in step 7 whenever you're done. https://github.com/b-/PVEThemes/tree/nord-develop

Note: substitute -b nord-develop instead of -b develop to grab a branch that's a merger of both my develop and the nord branches: git clone https://github.com/b-/PVEThemes -b nord-develop

@IgorIsaiasBanlian
Copy link

Please make this theme compatible with PBS (Proxmox Backup Server)!

@b-
Copy link
Author

b- commented Feb 4, 2023

Please make this theme compatible with PBS (Proxmox Backup Server)!

Hi, please see #136 and specifcally the second-to-last comment. There is a fork of the Discord Dark theme for PBS by another user on github (https://github.com/Luckyvb/PBSDiscordDark). I don't think PBS is nearly as popular as Proxmox VE, so I don't expect it to be maintained as much as this one (and @Weilbyte has stated they aren't planning to support PBE themselves).

You're more than welcome to fork that and update it and/or add the Nord colors, though! I'm sure a lot of people would appreciate it!

@Weilbyte
Copy link
Owner

Weilbyte commented Feb 5, 2023

Hi, thank you for the very in-depth PR.

The general idea of custom themes initially was to have something like a GitHub wiki which lists forks which have been modified with different colors. However, when giving this more thought it is not as secure since providing the installer a custom REPO and TAG essentially means that the JavaScript file will also be installed, thus if the fork has replaced the harmless code with malicious code, it will be executed. While on this topic, I am not aware of why supplying the REPO/TAG did not work for you, could you please try instead using the SHA-1 hash for the tag/branch instead?

As it stands the code is a bit of a mess, so when I find time I am going to first look if it can be restructured in a better way. If not, then a rewrite of the tooling (from shell script and python to single static executable) would take priority before themes. With this tool rewrite however, it would be feasible to have the theme compile locally, which should hopefully then allow more flexibility in custom themes (i.e., swap out vars file before compile, according to user selection). Let me know what you think of this approach.

As for the changes with the offline installation, I do agree that it would be better to have it work on detecting an offline install using the meta folder, in order to simplify the experience. At the current time though, please put in the issues in their own separate issues/PRs for easier tracking.

Happyrobot33 added a commit to Happyrobot33/PVEThemes that referenced this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants