Skip to content

Conversation

gordonwoodhull
Copy link
Contributor

@gordonwoodhull gordonwoodhull commented Oct 15, 2024

Couple questions for @cscheid here, so leaving this as draft:

  1. This implements the relative path to the brand yaml logo resources in core/brand/brand.ts. I think the implementation is good but I wonder if this belongs somewhere else? since I don't find the similar implementation for typography in this source file (but typography brand.yml-relative paths do work, as tested in this PR).

  2. I've been populating the brand key in the metadata with an object containing some of the processed brand yaml data, e.g. brand.typography.headings.family etc., for the template to consume.

    However, this may conflict when the metadata contains an explicit brand: path/brand.yml entry.

    Here I replace the string (Inlines) with an object, which feels problematic. Should we rename brand data to something else? You expressed the opinion that this data should be available to custom lua filters.

@gordonwoodhull gordonwoodhull force-pushed the feature/brand-yaml-relative-logo-paths branch 4 times, most recently from 1c22e37 to 24144b5 Compare October 15, 2024 21:40
@gordonwoodhull gordonwoodhull marked this pull request as ready for review October 16, 2024 18:53
@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Oct 16, 2024

As discussed, we can clean up the implementation of logo paths later, and replacing brand in metadata with an object is fine, as probably the path should not be emitted there anyway.

Will merge once tests pass.

i think there is a better place to do this, since fonts do this elsewhere
questionable, maybe we should be using a different metadata key here
@gordonwoodhull gordonwoodhull force-pushed the feature/brand-yaml-relative-logo-paths branch from 24144b5 to 0a02faa Compare October 16, 2024 18:56
@gordonwoodhull gordonwoodhull merged commit ee5a93c into main Oct 16, 2024
47 checks passed
@gordonwoodhull gordonwoodhull deleted the feature/brand-yaml-relative-logo-paths branch October 16, 2024 19:29
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.

1 participant