-
Notifications
You must be signed in to change notification settings - Fork 57
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
Create directory for themes #92
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to run the code for this change but it's not creating the theme files in the theme directory.
Theme directory is created successfully though.
I'm on macOS.
So it seems the issue is if you already have a config and have used the app before it doesn't go into this option in the main->load_from_state() function.
Which means that Config::create_themes_dir() is never called inside the Welcome::new() method. If we add Config::create_themes_dir() at the start of the load_from_state() function it adds the theme files in the themes directory but it doesn't update the app UI until the next time you open the app. |
You're right, if you already had a config file, themes files weren't being created. Moved Main issue is that a config file with |
Great PR @jhff. I think my reason behind this is that, as a user i would rather build a collection of themes i want, rather than a bunch of themes i never want to use. |
np, I'll rework this PR to remove those themes and leave the default one! It was just an initial approach with a set of hardcoded themes 🙂 |
Removed all themes except the default one ce6da49 Users can add their own theme files. Also expect "name" field in theme files so that we can list them in the future in the app if needed with the proper name. |
assets/themes/halloy.yaml
Outdated
@@ -0,0 +1,11 @@ | |||
name: "Halloy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need name
or can the filename just be the key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name of the theme as the value is more user friendly in my opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way to go is:
- filename -> config file theme key
name
-> theme name to display in the app
Let's say we add the file solarized_dark.yaml
for the Solarized (Dark)
theme. solarized_dark
is used as the key and Solarized (Dark)
can be used to display in the future command bar of the app, e.g..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR name
is not used anywhere, it's just there so users are already educated to add that instead of having to change their files in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should wiki be updated with name
in the example themes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should yeah, @jhff.
src/main.rs
Outdated
// Create themes directory | ||
Config::create_themes_dir(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: A module namespace works better here than struct namespace... config::create...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two last thing I have is that I would like to rename Otherwise I am good to merge. |
* Add themes folder to config directory * Add multiple themes * Load theme from config file at startup
... and add name to theme files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go!
This change addresses issue #65 by:
palette
. If theme is not found, defaultPalette
is used.