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

forge: init #573

Merged
merged 1 commit into from
Oct 7, 2024
Merged

forge: init #573

merged 1 commit into from
Oct 7, 2024

Conversation

brckd
Copy link
Contributor

@brckd brckd commented Sep 25, 2024

Adds border colors to the Forge GNOME extension. It basically makes all focused window borders blue. In contrast to the style guide, unfocused windows don't have a border, because I couldn't find a setting for that.
image

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: Approved-by: NAHO <[email protected]> .


The following comment is purely for reference for #534:

diff --git a/modules/forge/hm.nix b/modules/forge/hm.nix
new file mode 100644
index 00000000..9a9b1f51
--- /dev/null
+++ b/modules/forge/hm.nix
@@ -0,0 +1,13 @@
[...]
+  config = lib.mkIf (config.stylix.enable && config.stylix.targets.forge.enable) {
+    xdg.configFile."forge/stylesheet/forge/stylesheet".source = config.lib.stylix.colors {
[...]
+    };
+  };

As mentioned in #543, it would be good to reconsider in #534 what functionality should be enabled by default. Maybe config.lib.stylix.mkEnableTarget should consider whether the target itself is enabled.

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: Approved-by: NAHO <[email protected]>.

Actually, the modules/forge/stylesheet.css.mustache file should only declare things related to colors or fonts. There have been some discussions on standardizing border widths, but that will probably be standardized once the style guide has been formalized.

@brckd
Copy link
Contributor Author

brckd commented Sep 26, 2024

Thanks for the review!

Would something along the lines of config.lib.stylix.mkEnableTarget "Forge" (builtins.elem pkgs.gnome-extensions.forge options.programs.gnome.extensions) work?
I will also try to see whether I can just strip the border styling.

@trueNAHO
Copy link
Collaborator

Would something along the lines of config.lib.stylix.mkEnableTarget "Forge" (builtins.elem pkgs.gnome-extensions.forge options.programs.gnome.extensions) work?

Probably. However, it might be better to not include this in this PR as this is not the current Stylix convention for enabling targets. In the future, this should be addressed with an atomic tree wide patch.

I will also try to see whether I can just strip the border styling.

Essentially, most hard-coded (not relying on Stylix variables) options should be removed.

@brckd
Copy link
Contributor Author

brckd commented Sep 28, 2024

I tried simply removing the hardcoded values, but it raised an error in Forge.
image
My next option would be to create a home manager module to configure forge declaratively, but that will be quite the efford.

@brckd brckd force-pushed the forge/init branch 2 times, most recently from 900bf50 to f4c628e Compare September 28, 2024 20:42
Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My next option would be to create a home manager module to configure forge declaratively, but that will be quite the efford.

This is already an acknowledged template limitation:

the use of extraConfig might cause other potentially related issues. As discussed in #159 (comment), it makes it harder to override options, and as discussed in #388 (comment), it makes it easier to accidentally override Stylix options.

-- #395

In the future, I will resolve these limitations when cleaning up the codebase. So I don't mind leaving this as-is for now.

I tried simply removing the hardcoded values, but it raised an error in Forge.

Does Forge not have default values and requires certain values to be declared? Considering that Stylix is not entirely consistent with hard-coded values, I don't mind merging this as-is. @danth, what do you think?

Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to merge in its current state (and can be improved on later if necessary)

@danth danth requested a review from trueNAHO October 6, 2024 19:52
@brckd
Copy link
Contributor Author

brckd commented Oct 7, 2024

For transparency, I was just rebasing to master in the last couple of force pushes.

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test this, but LGTM:

Approved-by: NAHO <[email protected]>

@trueNAHO trueNAHO merged commit 63426a5 into danth:master Oct 7, 2024
10 checks passed
Mikilio pushed a commit to Mikilio/stylix that referenced this pull request Nov 4, 2024
Link: danth#573

Approved-by: Daniel Thwaites <[email protected]>
Reviewed-by: NAHO <[email protected]>
mateusauler pushed a commit to mateusauler/stylix that referenced this pull request Nov 17, 2024
Link: danth#573

Approved-by: Daniel Thwaites <[email protected]>
Reviewed-by: NAHO <[email protected]>
mateusauler pushed a commit to mateusauler/stylix that referenced this pull request Nov 20, 2024
Link: danth#573

Approved-by: Daniel Thwaites <[email protected]>
Reviewed-by: NAHO <[email protected]>
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.

3 participants