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

Minimal custom themes implementation #330

Merged
merged 22 commits into from Aug 7, 2023

Conversation

joshuamegnauth54
Copy link
Contributor

This is a much simpler patch than the last PR. Themes are implemented as plain structs directly in the code. They're automatically added to the settings page after the official themes as well.

I'm opening this as a draft so you can take a look at the code. I'll add 3-5 more themes later, but for now everything works so you can try it or review the source.

The new types are to avoid mangling the current style implementation too
much.
For now, custom themes will use constant structs like in Sniffnet's
original code. Custom themes will be self contained instead of polluting
the original code base. This should make it easier for contributors to
modify or add any extra themes without having to change code in too many
files.
I implemented a theme to ensure there are no compiliation issues.
Floating point math can't be called from a const context, so the themes
are returned from a non-const function for now. `iced::color!` is used
for ergonomics.
* Support appending extra styles to the settings page AFTER the built in
  styles.
* Make the whole thing scrollable.
@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 2, 2023

Tyvm!
I have to merge a pretty big PR that could break something first (#324).
After that, I'll start reviewing and introducing some changes I have in mind to discuss with you.

@GyulyVGC GyulyVGC added the design Styling and appearance label Aug 2, 2023
@GyulyVGC GyulyVGC added this to the v1.2.2 milestone Aug 2, 2023
@joshuamegnauth54
Copy link
Contributor Author

No problem. Luckily, this PR is very small and easy to maintain.

`StyleType::Custom` should be serialized to and deserialized from a TOML
table. The `toml` crate doesn't seem to figure this out which causes
saving and loading the settings to fail.

I fixed this by hinting that the type should be a table using serde's
attributes.
@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 3, 2023

I've merged #324 🎉
Feel free to solve conflicts but don't feel forced to, I'll review this PR in some days so I can do it myself in case you're busy.

@joshuamegnauth54
Copy link
Contributor Author

I merged in the latest changes. 😺 There weren't many conflicts - just a few imports and updating the code to handle the new is_nightly function.

src/gui/styles/types/custom_styles.rs Outdated Show resolved Hide resolved
src/gui/styles/types/custom_styles/dracula.rs Outdated Show resolved Hide resolved
@joshuamegnauth54
Copy link
Contributor Author

In terms of extra styles, I'm using bat's implemented themes as a guide. I'll add a few more, but we can figure out which to keep or add later.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 5, 2023

In terms of extra styles, I'm using bat's implemented themes as a guide. I'll add a few more, but we can figure out which to keep or add later.

Sounds good.
Ideally, I'd like to drop a new release in a few days and merge this PR in short time.
It'd be nice to have an even amount of total themes and dark themes equal to light ones, so that we can keep all the dark on the left and viceversa.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 5, 2023

Changes

I've generalised the colors for round borders and containers and now it's sufficient to specify their opacity.
I've also merged the opacity of badges and charts in a single parameter.

Improvements

Some of the palettes required changes to be more aesthetically pleasing in my opinion, particularly Solaraized (Day) because green text wasn't clearly readable.
Feel free to share your ideas though.
Color gradients are computed in an automatic way and look good to me.

Notes

Nord theme is amazing.

I'm planning to introduce for each Night theme its Day counterpart.
To this purpose, I'll add Gruvbox Day and Nord Day.

The only theme without a bright counterpart would be Dracula.
Do you know by chance a bright theme we could use to pair with Dracula?
It should be a theme without a dark counterpart itself.

Edit

For this PR, I think 8 additional themes are sufficient.
Let me know your ideas about the 5 new themes already defined.
The missing ones are:

  • Gruvbox (Day)
  • Nord (Day)
  • An exclusively day-based theme to be counterpart of Dracula (if a Dracula Day theme doesn't exist as I suspect)

Edit 2

It'd be amazing to also get Sniffnet featured on the official pages of the themes we are using

@joshuamegnauth54
Copy link
Contributor Author

joshuamegnauth54 commented Aug 6, 2023

You covered a lot of the questions I intended to ask today before I asked them. 😂

Improvements

I'm not great at aesthetics, so you can definitely tweak the styles in any way you see fit. I also like the color gradients a lot!

Notes

Yes, Nord is amazing. I'm pleased with how the style turned out as well. The official Nord site has excellent documentation too which made it easier to implement the style. Also, a lot of programs implement the theme, so you can probably use it for other programs if you want to as well.

I added light versions of Gruvbox and Nord. As for Dracula, I'll look into a light style that looks kind of similar. The official Dracula site has an FAQ on whether there will be a light theme - and the answer is "Nope. Dracula can't stand the light." It's clever. 😂

As for featuring Sniffnet on the themes' sites, I think most of them allow you to open a PR or issue to add it. Some themes, like Solarized, don't have an official list or anything. However, solarized is well-known and a lot of people love it.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 6, 2023

As for Dracula, I'll look into a light style that looks kind of similar. The official Dracula site has an FAQ on whether there will be a light theme - and the answer is "Nope. Dracula can't stand the light." It's clever. 😂

That's pretty hilarious, legit.

As for featuring Sniffnet on the themes' sites, I think most of them allow you to open a PR or issue to add it. Some themes, like Solarized, don't have an official list or anything. However, solarized is well-known and a lot of people love it.

I'm waiting for the approval from the Dracula team (I sent a request this morning), the other teams repos seem pretty inactive and have lots of requests opened since months... I don't think it's worth to try with them...


Btw... I finally understood why highlighted text was not clearly visible.
Its color was conditionally generated based on the body text being black or not.
I've updated it using the is_nightly method and now everything looks hella clearer.
I've also switched back Solarized Day to use green since it's lot cooler now that text is well visible.

@joshuamegnauth54
Copy link
Contributor Author

Oh yeah, some of the themes don't really publish or update their lists. Dracula, Nord, and Catppuccin seem to keep their lists fairly updated.

I have a question about the eight themes. Do you mean eight theme pairs or eight themes total?

In other words, does "Nord (Day)" and "Nord (Night)" count as one theme (pair) or two?

For Dracula's day theme, I found this edited port which was linked on GitHub. It uses Dracula's colors for a light theme as well. I'll see if I can port it over.

@joshuamegnauth54 joshuamegnauth54 marked this pull request as ready for review August 7, 2023 03:22
@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 7, 2023

I have a question about the eight themes. Do you mean eight theme pairs or eight themes total?

Eight in total, so we should be done now!

@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 7, 2023

Thank you very much for your time on Sniffnet 🙏
I hope we'll collaborate again in the future!

@all-contributors please add @joshuamegnauth54 for code, design.

@allcontributors
Copy link
Contributor

@GyulyVGC

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @joshuamegnauth54! 🎉

@GyulyVGC GyulyVGC changed the base branch from main to v1.2.2 August 7, 2023 15:13
@GyulyVGC GyulyVGC merged commit e6c75e7 into GyulyVGC:v1.2.2 Aug 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Styling and appearance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants