feat(theme-classic): allow specifying width/height in logo

[cerkiewny]

Motivation

Fixes #5724

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Check manually if new config works,
snapshot tests for website should not change with new config

[netlify]

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 55c68e3

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/617fba3a9593f100078960e6

😎 Browse the preview: https://deploy-preview-5770--docusaurus-2.netlify.app

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟢 Performance	99
🟢 Accessibility	98
🟠 Best practices	86
🟢 SEO	100
🟢 PWA	91

Lighthouse ran on https://deploy-preview-5770--docusaurus-2.netlify.app/

[cerkiewny]

Let me know what you think about code change I will follow it up with doc change if the code changes are in the right direction.

[slorber]

Thanks

In any case, we don't want to change the current API and avoid any breaking change.

Using a string remains the simple option for the end user, and Docusaurus should later handle it automatically by transforming it to an ImageObject: it should not be done by the user (or at least it should not be mandatory).

I suggest we could just forward additional props to the img element directly, and write in our own website config:

module.exports = {
  themeConfig: {
    navbar: {
      title: 'Site Title',
      logo: {
        src: 'img/logo.png',
        width: 50,
        height: 50,
        className: "my-custom-logo"
      },
    },
  },
};

We'll figure out later how to automatically infer a good default width/height for the referenced FS image

[cerkiewny]

Thanks

In any case, we don't want to change the current API and avoid any breaking change.

Using a string remains the simple option for the end user, and Docusaurus should later handle it automatically by transforming it to an ImageObject: it should not be done by the user (or at least it should not be mandatory).

I suggest we could just forward additional props to the img element directly, and write in our own website config:

module.exports = {
  themeConfig: {
    navbar: {
      title: 'Site Title',
      logo: {
        src: 'img/logo.png',
        width: 50,
        height: 50,
        className: "my-custom-logo"
      },
    },
  },
};

We'll figure out later how to automatically infer a good default width/height for the referenced FS image

Not sure if it is/isn't a problem but in this case width/height would be shared between logo from src and srcDark.

[slorber]

Not sure if it is/isn't a problem but in this case width/height would be shared between logo from src and srcDark.

That does not seem like a big deal: users should use logos of same size to avoid layout shifts anyway, and has the responsibility to not mess things up: we just pass through, no additional logic

[cerkiewny]

Not sure if it is/isn't a problem but in this case width/height would be shared between logo from src and srcDark.

That does not seem like a big deal: users should use logos of same size to avoid layout shifts anyway, and has the responsibility to not mess things up: we just pass through, no additional logic

Well had more of responsive logo idea in mind then src/srcDark really, but I guess if there is no plan for it, it can be added later

[slorber]

Well had more of responsive logo idea in mind then src/srcDark really, but I guess if there is no plan for it, it can be added later

Not sure what you mean, but 2 logos with different widths means clicking on the theme switch will shift navbar items: bad UX

[cerkiewny]

One missing thing in this PR is the documentation change to describe config for width/height @Josh-Cena

[Josh-Cena]

Sure, thanks :)

[Josh-Cena]

I think this is good enough for now. We can try:

• Allow passing arbitrary props to logo images, just like for navbar links;
• Infer image sizes from files

But those can be in follow-ups. This solution is fine.

[cerkiewny]

I still think there is usecase for responsive logos, I would wait for the "PlatformSpecific" feature to be implemented, and then we could add platform specific to the logo setting to support responsive logo usecase if theme supports platforms like high DPI phones. Here are examples of "responsive logos":

[Josh-Cena]

I still think there is usecase for responsive logos, I would wait for the "PlatformSpecific" feature to be implemented, and then we could add platform specific to the logo setting to support responsive logo usecase if theme supports platforms like high DPI phones. Here are examples of "responsive logos":

Ah, yes, we could look into that. We don't want logos to be responsive to light/dark, but screen sizes sound fine. Beware though that currently all logo usages are scaled so that their sizes are invariant on all screen sizes.

[slorber]

Maybe we could allow passing a className so that you can handle the responsiveness yourself with CSS?

I think lighthouse might still report an error though, and you'll need to keep the width/height

[Josh-Cena]

I think lighthouse might still report an error though, and you'll need to keep the width/height

width and height are needed for minimizing layout shifts (while we still have many layout shifts with other images), and I do think we should allow any props passed to the rendered image. From @cerkiewny's demo I think they actually want different images being rendered, which IMO is not necessary for now

[alex-drocks]

Logo is broken when updating from beta8 to beta9 on header and footer. I tried adding width and height but it still cannot find the image that was already working in beta8.

[Josh-Cena]

@alex-drocks this is a different issue. If you are using a base URL you are probably experiencing #5863, which has been fixed in a canary release.

[alex-drocks]

@alex-drocks this is a different issue. If you are using a base URL you are probably experiencing #5863, which has been fixed in a canary release.

Thanks Josh, yes in fact I am using a baseUrl.

For reference to other readers, it means i am not using the default localhost:port/ base but something like localhost:port/my-base-path/...