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

brand yaml logo feature parity: dashboard, revealjs #11446

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

gordonwoodhull
Copy link
Contributor

@gordonwoodhull gordonwoodhull commented Nov 15, 2024

Replaces #11445

Implements missing feature/format combinations for dashboard, revealjs, i.e. the Xs in the first two rows of this matrix:

image

I will continue with the website Xs #11309 #11310 but those are a tiny bit more complicated because I think logo resolution needs to happen later:

const projectBrand = await project.resolveBrand();
if (
projectBrand?.processedData.logo && sidebars?.[0]
) {
if (sidebars[0].logo === undefined) {
const logo = projectBrand.processedData.logo.medium ??
projectBrand.processedData.logo.small ??
projectBrand.processedData.logo.large;
if (logo) {
sidebars[0].logo = logo.light.path; // TODO: This needs smarts to work on light+dark themes
sidebars[0]["logo-alt"] = logo.light.alt;
}
}

It is my understanding that project.resolveBrand() should never be called without a target, but we don't have one at this point.

Notes:

  • revealjs does not currently have alt text for the logo, so I did not implement it.
  • no support for dark mode - I think this would be easy but I haven’t looked.

allowing logo override with path or resource name
also allowing logo object with path containing one of those and opt alt text
or object containing path or resource name
@gordonwoodhull gordonwoodhull merged commit 6b2db81 into main Nov 15, 2024
47 checks passed
@gordonwoodhull gordonwoodhull deleted the feature/brand-yaml-logo-feature-parity branch November 15, 2024 02:58
@cscheid
Copy link
Collaborator

cscheid commented Nov 15, 2024

project.resolveBrand() without a target

FWIW, it takes an optional fileName; when missing, it only attempts to resolve the project-level brand information.

@gordonwoodhull
Copy link
Contributor Author

I said that too strongly.

Yes, it is currently fetching project-level brand data correctly. The problem is that project-level brand data may be unexpected since we have a couple of ways to assign document-level brand data.

Today I’m going to try loading the logo at document level, to allow embedded brand data and document choice of logo. If it’s not possible or causes problems I’ll give up and accept the inconsistency.

@cscheid
Copy link
Collaborator

cscheid commented Nov 15, 2024

That inconsistency is not a show-stopper by any means.

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.

2 participants