Skip to content

stylix: simplify font options & use literalExpression default packages#1225

Merged
danth merged 2 commits intonix-community:masterfrom
MattSturgeon:font/defaultText
May 9, 2025
Merged

stylix: simplify font options & use literalExpression default packages#1225
danth merged 2 commits intonix-community:masterfrom
MattSturgeon:font/defaultText

Conversation

@MattSturgeon
Copy link
Copy Markdown
Member

@MattSturgeon MattSturgeon commented May 4, 2025

Derivation values shouldn't be rendered in option docs. Instead a literalExpression or literalMD should describe the default/example.

This PR was cherry-picked from #1212 in an effort to make that PR smaller and easier to review, while also making these changes easier to focus on in a dedicated PR.


I've gone a step further than the minimal fix originally proposed in #1212, as I believe the options can be simplified a little:

  1. I'm not convinced each font option needs to be a submodule
  • they could just be a set of nested options
  • submodules are more useful when there is compartmentalised logic or reusable types
  1. I'm not convinced the defaults are best defined/documented at the top-level font option instead of at the sub-options (name and package)
  2. Pushing down the defaults allows using lib.mkPackageOption instead of implementing defaultText it by hand.

I believe this also results in simpler documentation, with two entries per font instead of three:

Screenshot

image

Things done

Notify maintainers

@dwarfmaster

@MattSturgeon MattSturgeon changed the title stylix: use literalExpression for font package defaults stylix: simplify font options & use literalExpression default packages May 4, 2025
Copy link
Copy Markdown
Contributor

@0xda157 0xda157 left a comment

Choose a reason for hiding this comment

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

LGTM

Derivation values shouldn't be rendered in option docs.

Instead a `literalExpression` or `literalMD` should describe the
default/example.

Usually, `lib.mkPackageOption` is convenient, however in this case the
option is a little more complex.
Copy link
Copy Markdown
Member

@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.

LGTM, thanks :))

@danth danth merged commit b06c1cb into nix-community:master May 9, 2025
4 checks passed
MrSom3body pushed a commit to MrSom3body-contrib/stylix that referenced this pull request May 9, 2025
@MattSturgeon MattSturgeon deleted the font/defaultText branch May 9, 2025 12:09
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