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

feat!: flavor iteration, rework --overrides & add examples #111

Merged
merged 33 commits into from
Feb 8, 2024

Conversation

sgoudham
Copy link
Contributor

@sgoudham sgoudham commented Dec 16, 2023

Still some outstanding questions / design decisions to hash out.

I've added a new nested.hbs file that does currently render, but I'm not quite sure how frontmatter should be handled when passing in the case of the all argument. I don't think we want to treat the entire file as being under the {{#each}} nested object but maybe we do?

BEGIN_COMMIT_OVERRIDE
chore(whiskers): experiment with overrides & flavour iteration
END_COMMIT_OVERRIDE

@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 16, 2023

From further discussion on discord with @nullishamy, I've also found that using HashMap here doesn't guarantee any order (which makes sense)

This isn't great for us because I'd imagine we always, if not most of the time, want to control the ordering of how the flavours and colours are listed. With the hashmap implementation, I'm always getting Frappe at the start instead of Latte on READMEs.

Potential solutions might be to use Vecs and/or ordered Hashmaps?

Goal

The following handlebars syntax:

{{#each flavors}}
- **{{flavor}}**
  {{ #each colors as | color colorName | }}
  - **{{colorName}}**
  {{/each}}
{{/each}}

should produce the same order of flavours & colours as the main catppuccin/catppuccin table

@nekowinston
Copy link
Contributor

Yeah, I noticed that the order wasn't guaranteed, #104 was pretty rushed to just add the basic functionality without spending much time doing Rust.
IIRC I used BTreeMap from stdlib for ordered stuff before, but I'll trust your judgement with the Rust implementation.

As for the frontmatter, it would probably make sense if we enforced explicit color overrides, like in VSCode/nvim etc, e.g.

colorOverrides:
  mocha: {}
  macchiato: {}
  frappe: {}
  latte: {}
  all: {}

The other frontmatter can probably be safely assumed to be global, since isLight/isDark are utility functions, rather than data.

@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 16, 2023

@nekowinston I did have a look at BTreeMap but I didn't find anything that would allow me to pass in a custom sorter for the colors, and therefore opted for the IndexMap. Let me know if I missed a function like that though.

When you say explicit color overrides, do you mean removing them as a command line flag and making them only available in the frontmatter?

Also what I meant in my initial comment is that I'd still like to alias colours to variable names so that it can act like documentation.

E.g. With the new changes, I can configure the Slack port as follows:

---
weird: "F8F8FA"
---
{{#each flavors}}
- **{{titlecase flavor}}**{{#if @first}} (Make sure to enable light mode!){{/if}}
  \`\`\`
  #{{uppercase base}},#{{../weird}},#{{uppercase mauve}},#{{uppercase base}},#{{uppercase crust}},#{{uppercase text}},#{{uppercase mauve}},#{{uppercase maroon}},#{{uppercase base}},#{{uppercase text}}
  \`\`\`
{{/each}}

But I'd prefer an approach similar to how the slack port currently looks where the variable names are mapped to the colours in the frontmatter:

---
column_bg: "{{base}}"
weird: "F8F8FA"
active_item: "{{mauve}}"
active_item_text: "{{base}}"
hover_item: "{{crust}}"
text_color: "{{text}}"
active_presence: "{{mauve}}"
mention_badge: "{{maroon}}"
top_nav_bg: "{{base}}"
top_nav_text: "{{text}}"
---
{{#each flavors}}
- **{{titlecase flavor}}**{{#if @first}} (Make sure to enable light mode!){{/if}}
  \`\`\`
  #{{uppercase column_bg}},#{{../weird}},#{{uppercase active_item}},#{{uppercase active_item_text}},#{{uppercase hover_item}},#{{uppercase text_color}},#{{uppercase active_presence}},#{{uppercase mention_badge}},#{{uppercase top_nav_bg}},#{{uppercase top_nav_text}}
  \`\`\`
{{/each}}

The approach I've defined is impossible, at the minute, as the frontmatter is rendered before the template itself. Of course, when you set it the FLAVOR to all, there is no global {{base}}.

I suppose what I'd be interested is maybe a helper of some sort to allow this mapping/aliasing of a variable name to a colour?

@nekowinston
Copy link
Contributor

@nekowinston I did have a look at BTreeMap but I didn't find anything that would allow me to pass in a custom sorter for the colors, and therefore opted for the IndexMap. Let me know if I missed a function like that though.

I wrote that comment before I saw your indexmap commit, consider that resolved 🙃

When you say explicit color overrides, do you mean removing them as a command line flag and making them only available in the frontmatter?

Yeah, since you can still set them via JSON in stdin / JSON file arguments.
I'd like it if we could have a single option that works the same across all templated ports, whether they're using flavor iteration or not; so explicitly having a single colorOverrides object would help with that.

Also what I meant in my initial comment is that I'd still like to alias colours to variable names so that it can act like documentation.

That makes sense, terminal ports are a good use case here as well; they can share a common frontmatter like

bg: {{ base }}
fg: {{ text }}
sel: {{ surface2 }}
color0: {{ darklight subtext0 surface1 }}
color1: {{ red }}
color2: {{ green }}
color3: {{ yellow }}
color4: {{ blue }}
color5: {{ pink }}
color6: {{ sky }}
color7: {{ darklight subtext1 surface2 }}
color8: {{ darklight surface2 subtext0 }}
color9: {{ red }}
color10: {{ green }}
color11: {{ yellow }}
color12: {{ blue }}
color13: {{ pink }}
color14: {{ sky }}
color15: {{ darklight surface1 subtext1 }}

That would require dealing with the current flavor context as well, though.

@sgoudham
Copy link
Contributor Author

@nullishamy and I were discussing overrides earlier and we failed to see how the overrides object could be defined in the frontmatter for single flavours. i.e.

Given this command here in lazygit:

@{{whiskers_cmd}} {{template_path}} {{flavor}} --override accent="{{accent}}" > {{output_base}}/{{flavor}}/{{accent}}.yml

The --override accent= flag allows for the template to specify a placeholder variable that can be overridden as each accent colour is passed in. We weren't really sure how this model could work if all the overrides were specified in the template itself?

I did mention that it feels a bit odd to rely on the overrides flag to generate files with different accents. There's something in me that feels odd about it but maybe that's just me.

@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 20, 2023

The latest commit that I've just pushed (9759473 (#111)) allows the frontmatter to be parsed for every single flavour, meaning that we can now successfully recreate the markdown table with every flavour and colour in the correct order.

However, there's still some things to hash out / do:

  • How do we want to handle command line overrides for all flavours?
    • I've added support for doing command line overrides into the root level context but really don't want to bother with parsing command line overrides when it's multiple flavors, that's where the yaml approach would come in handy. Maybe we should just disallow command line overrides when flavor is set to all ?
  • As the entire frontmatter is being parsed under each the context of each flavour, no frontmatter can be defined in the root context. We need to be able to detect if the frontmatter variable doesn't include {{ }} - if it doesn't then we can avoid duplicating it across each flavour.
  • Write tests
  • refactor code to be nicer/tidier (still a rushed implementation atm while trying to match the features from the single flavours)

@nekowinston
Copy link
Contributor

nekowinston commented Dec 21, 2023

I've added support for doing command line overrides into the root level context but really don't want to bother with parsing command line overrides when it's multiple flavors, that's where the yaml approach would come in handy. Maybe we should just disallow command line overrides when flavor is set to all?

Yeah

would probably make sense if we enforced explicit color overrides, like in VSCode/nvim

in vscode, the overrides are applied like this:
https://github.com/catppuccin/vscode/blob/3cb3529c5c22ea47203f5e0e6e357b6b2623cfce/src/theme/index.ts#L39-L43

Taking the library palette, combining it with all: first, then the flavor. So

{
  "colorOverrides": {
     "all": {
       "base": "#000000"
     },
     "latte": {
       "base": "#ffffff"
     }
   }
 }

would result in Mocha, Macchiato, Frappe having a black background while Latte keeps white.

whiskers --override '{"colorOverrides":{"all":{"base":"#000000"},"latte":{"base":"#ffffff"}}}'

This should work regardless of how the template is built, single flavor per file, multiple flavors per file.

@sgoudham
Copy link
Contributor Author

I'm still unsure on how overrides for single flavours could be defined in the yaml as specified in #111 (comment) @nekowinston

The lazygit port essentially relies on a placeholder variable to be overridden on every single invocation for all the accents, having it defined in the yaml doesn't make sense for how that is currently implemented.

@sgoudham
Copy link
Contributor Author

Iterating over accents like that almost feels like it should be handled by more of a build system/task runner (which it technically is with the justfile) but it also feels weird that the way to generate multiple accent files with whiskers right now is basically with placeholder variables.

@nekowinston
Copy link
Contributor

nekowinston commented Dec 21, 2023

whiskers mocha means that Whiskers is already in the mocha flavor context.

---
accent: "{{ blue }}"
---
theme:
  activeBorderColor:
    - '#{{ accent }}'

"{{ blue }}" is defining the default here; whiskers mocha --override "{"accent": "{{ pink }}"} would use mocha.pink.

@nekowinston
Copy link
Contributor

Iterating over accents like that almost feels like it should be handled by more of a build system/task runner (which it technically is with the justfile) but it also feels weird that the way to generate multiple accent files with whiskers right now is basically with placeholder variables.

I do agree tho that I'd prefer if we didn't have to write absolutely cursed build files, this justfile seems like a step down from saner build scripts. I'd like it if whiskers could handle the "build" similar to how other build systems deal with <cpu_architecture>-<operating-system> matrices.

@sgoudham
Copy link
Contributor Author

Agreed, although I think we're doing a good job of trying to integrate it in a variety of different settings and seeing the pain points so we can iteratively improve the design. #112 should really help with this.

@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 21, 2023

I think you can actually omit all in the overrides yaml because the {{}} variables act like the all context anyways.

I'm going to try and get the following syntax parsed for the single file support:

---
# Set default accent color
accent: "{{mauve}}"
# Set custom variables
user: "@sgoudham"
overrides:
  # And you can override variables for specific flavors
  frappe:
    name: "frappé"
    accent: "{{blue}}"
  mocha:
    accent: "{{green}}"
    user: "@nekowinston"
---

Unfortunately it's going to probably going to take some trial and error for me to get it right lol

I think it's honestly better ignoring the differences between "root" and "flavour" contexts when it's in the single file because it's such a headache to keep track of. Even if that means that a variable defined at the root level is duplicated into each of the flavours when it's not necessarily going to be used at the flavour level (this model is much better for updating the context when passing overrides anyways)

@nekowinston
Copy link
Contributor

nekowinston commented Dec 21, 2023

No my point is that --overrides just get merged with the frontmatter as-is, which reduces complexity, hence a single all override makes sense if we're going with that, IMO.

@sgoudham
Copy link
Contributor Author

I suppose the con is that you might potentially have to write out a long JSON string when calling whiskers, but somewhat mitigated by common tools like jq / yq ?

@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 22, 2023

I suppose I'm just confused at a potential situation in the frontmatter like below:

---
# Set default accent color
accent: "{{mauve}}"
# Set custom variables
user: "@sgoudham"
overrides:
  all:
    accent: "?"
  # And you can override variables for specific flavors
  frappe:
    name: "frappé"
    accent: "{{blue}}"
  mocha:
    accent: "{{green}}"
    user: "@nekowinston"
---

You wouldn't define the all override if you're already in the single file mode... right?

@nekowinston
Copy link
Contributor

nekowinston commented Dec 22, 2023

I suppose the con is that you might potentially have to write out a long JSON string when calling whiskers

At that point you're probably customizing a theme rather than authoring it, I'd hope; otherwise our build tool is pretty bad 🙃
The upside here is that it's at least consistent across ports, I'm mainly thinking of Nix here, e.g. whiskers --override '${builtins.toJSON myoptions}'.

That's why I said

I'd like it if whiskers could handle the "build" similar to how other build systems deal with <cpu_architecture>- matrices.

I'd love if we could also get generating multiple accents working later, with standardized, generated filenames.

You wouldn't define the all override if you're already in the single file mode... right?

Sure; like with your slack color example that just needs to stay the same across all flavors for some reason.

@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 22, 2023

I'm mainly thinking of Nix here,

You? The most nix-pilled user on GitHub? Thinking of nix? I am shaken to my very core

Context

image

Yeah I think a lot of my confusion came from the fact that we're wanting to keep the frontmatter the same even though the overrides yaml block would only be valid in single file mode vs the other modes. I'll have a go at reworking all the overrides stuff to take in a JSON string and merging that with the existing frontmatter since it's only taking in key/value pairs atm.

@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 24, 2023

Just have been writing documentation and I've only now just realised that we don't actually need the all override at all, because the frontmatter root context is basically the all override already.

Will aim to push everything up soon you can understand it via the README 👍

@sgoudham sgoudham changed the title feat: add flavor iteration with new option all feat!: add flavor iteration, support --overrides, add examples & update docs Dec 25, 2023
@sgoudham sgoudham marked this pull request as ready for review December 25, 2023 00:08
@sgoudham sgoudham linked an issue Dec 25, 2023 that may be closed by this pull request
@sgoudham
Copy link
Contributor Author

sgoudham commented Dec 25, 2023

As discussed on discord with @nekowinston, the current overrides are always in the context of the current flavour, and this may not be the exact behaviour that we want.

However, this PR puts us in a better place than we were before so should be good to get reviewed and merged now 👍 (might be good to tag and release as a v2 alpha?)

Half of the files modified are new examples so it's best to just ignore that directory in the file view and jump to reviewing the *.rs files.

Copy link
Contributor

@nullishamy nullishamy left a comment

Choose a reason for hiding this comment

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

LGTM. Would have some notes about style/architecture but given that this is still very early stages, I think we can leave it for future refactoring.

Copy link
Contributor Author

@sgoudham sgoudham left a comment

Choose a reason for hiding this comment

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

This PR has marinated long enough, will merge and we can continue to iterate on it.

@sgoudham sgoudham changed the title feat!: add flavor iteration, support --overrides, add examples & update docs feat!: flavor iteration, rework --overrides & add examples Feb 8, 2024
@sgoudham sgoudham merged commit bf01699 into main Feb 8, 2024
3 checks passed
@sgoudham sgoudham deleted the feat/flavour-iteration branch February 8, 2024 00:13
@github-actions github-actions bot mentioned this pull request Mar 31, 2024
sgoudham added a commit that referenced this pull request Jun 3, 2024
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.

Better support for all flavours to single file
3 participants