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

Feature/theming #296

Merged
merged 6 commits into from
Oct 20, 2024
Merged

Conversation

Theoreticallyhugo
Copy link
Contributor

@Theoreticallyhugo Theoreticallyhugo commented Sep 26, 2024

the idea here is to allow fully custom colours, both in the hex-code, as well as the name and number of available colours, whilst keeping changes minimal.

i created this in an attempt to slowly but surely recover and go towards the goal of #224.

changes are documented on the documentation branch im working on, including some info to use other themes with dracula

what changed:
the colour variables now live in a separate file, which is sourced per default. a new colours.sh file can be created where the user wants ( for example ~/.config/tmux/colours.sh) and then sourced with the option set -g @dracula-colors "$current_dir/colors.sh"

so, for default users theres very little changes under the hood and none that they could observe.
however this little changes opens up many possibilities for customisation.

@ethancedwards8
Copy link
Member

I'm going to close this in favor of #224

I would appreciate any feedback you had on that PR.

@Theoreticallyhugo
Copy link
Contributor Author

I'll look into it. however I'm somewhat saddened with the lack of discussion of pros and cons between #294 and #296 😅

@ethancedwards8
Copy link
Member

It appears to be more flexible. I don't like the idea of users having to edit the programs script files. Modifying the files directly often inhibits updating the plugin, due to git detecting changes. Editing the .tmux.conf file removes this issue.

@Theoreticallyhugo
Copy link
Contributor Author

Theoreticallyhugo commented Oct 10, 2024

but thats an issue i kept in mind and actually the reason i created this solution in the first place. this would allow a user to create a file with whatever variables in any location on their machine. so, if a user had their tmux.conf under vcs, they could put the colours.sh file in the same dir and point the tmux option to that file.
basically the idea is to have one tmux option to set all colours at once, without having to touch the programs code at all. this could also be achieved by a long string appended to the tmux option, but would be quite ugly to read. hence the external file solution.
basically #294 s solution is to have a separate tmux option for each and every colour, whilst i wanted to reduce that to exactly one.

@ethancedwards8
Copy link
Member

ethancedwards8 commented Oct 10, 2024

That is a very good point. I did not consider having the file outside of the plugin directory. I just assumed that it would be the one to be edited.

I think I would be more open to this idea if instead of having a file in the directory, which can be misleading, we leave the default colors in dracula.sh but then overwrite them if the script detects the presence of a configuration file. That way, users are not tempted to edit the colors.sh file in the plugin repo, causing issues with updating. We could also point out that putting the custom color file in VCS could be beneficial to some users.

We would also need to provide an example configuration (essentially what you currently have in colors.sh right now) in INSTALLATION.md. I would not merge this without it.

Also, could you think of a solution where users are able to place colors into .tmux.conf instead of having to create an entirely new file just for colors?

@Theoreticallyhugo
Copy link
Contributor Author

i like the idea with keeping the default colours and just overriding them. i realised earlier today that not doing it that way may lead to weird issues and crashes when a user forgets assigning a variable that is used somewhere 😅

im gonna look into solutions, where the info could be placed inside the tmux.conf.

besides ive been working on configuration explanations/ installation instructions/ templates for themes in my documentation branch. i know a few people who would love to use this plugin if they could use it with guvbox or catppuccin and so im working on default configs that can be copied.

i can make sure to put instructions into installation.md for now, but im also assuming that it may move when the bigger documentation is getting ready to get merged.

i'm very happy that this solution may get merged. i've been hoping for this day for the past two years 😅

reabasing theming branch onto changes from master
@Theoreticallyhugo Theoreticallyhugo marked this pull request as draft October 11, 2024 14:07
@Theoreticallyhugo
Copy link
Contributor Author

converting this to a draft, for this is currently work in progress.
i've added info to the readme and install.md, pointing to the instructions in the docs dir.
i'm currently testing the catppuccin values, and have asked a friend to check gruvbox, as he knows that better than me.
both of those plug and play configs are configured without #224 ofc. we could consider implementing #224 in this pr, or wait with this pr till the #224 successor is merged, in order to finalise documentation on color theming. for that id like to hear your thoughts @ethancedwards8.

im still looking for a solution that would allow us to configure everything in tmux.conf.

INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
scripts/colors.sh Outdated Show resolved Hide resolved
scripts/dracula.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/color_theming/README.md Show resolved Hide resolved
@ethancedwards8
Copy link
Member

converting this to a draft, for this is currently work in progress. i've added info to the readme and install.md, pointing to the instructions in the docs dir. i'm currently testing the catppuccin values, and have asked a friend to check gruvbox, as he knows that better than me. both of those plug and play configs are configured without #224 ofc. we could consider implementing #224 in this pr, or wait with this pr till the #224 successor is merged, in order to finalise documentation on color theming. for that id like to hear your thoughts @ethancedwards8.

im still looking for a solution that would allow us to configure everything in tmux.conf.

Thanks for adding documentation.

Generally, it is advisable to do one thing at a time in FOSS projects. It keeps Git history clean, and makes it easier on maintainers and users to digest what is happening. For that reason, I think we should keep #224 and this PR strictly separate.

I'm not sure that the individual theming of each widget (git, kubernetes, terraform, etc) is necessary? Doesn't just setting the global colors change the widget colors as well?

@Theoreticallyhugo
Copy link
Contributor Author

i saw that you commented and will look into fixing later.

the individual theming of each widget is a feature that is not added by this pr and which i still consider important. there are cases in which multiple widgets use the same color by default, which is kinda ugly/ weird when they're next to each other. take for example all network widgets, which come in the same colors. global theming would change the color of each widget in the same way, which wouldn't solve the issue of indistinguishability.
besides, it is a very nice feature for a color theme and we do already have it implemented.

@Theoreticallyhugo
Copy link
Contributor Author

@ethancedwards8 i fixed the comments you had and changed the code to get rid of the external file.
i'm still working on documentation for the plug n play themes (only catppuccin and gruvbox so far), but the theming option itself should be fully functional now. will do some testing

@ethancedwards8
Copy link
Member

Everything here looks really good. I've thought about everything for a few days, and I can't really say that there is anything I would change. I am open to hearing feedback from other contributors.

@Theoreticallyhugo
Copy link
Contributor Author

awesome! ill add more prebuilt theme stuff when we have merged a pr that allows for more customisation :D

@Theoreticallyhugo Theoreticallyhugo marked this pull request as ready for review October 20, 2024 19:34
@ethancedwards8 ethancedwards8 merged commit 068a49f into dracula:master Oct 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants