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

Footer logo styling artifically restricts real uses #260

Closed
zpao opened this issue Jun 16, 2022 · 5 comments · Fixed by #262
Closed

Footer logo styling artifically restricts real uses #260

zpao opened this issue Jun 16, 2022 · 5 comments · Fixed by #262

Comments

@zpao
Copy link
Contributor

zpao commented Jun 16, 2022

The max-width specified here doesn't support short & wide logos well. Specifically it looks pretty bad when trying to use the new Meta Open Source logo.

&__logo {
margin-top: 1rem;
max-width: 10rem;
}

image

This is then not easy to override inside Docusaurus. The width property you can specify in the logo config isn't respected if greater than 10rem.

You could override with inline style here (https://github.com/facebook/docusaurus/blob/6df379ca6f9510793da73183acc851b653575e6a/packages/docusaurus-theme-classic/src/theme/Footer/Logo/index.tsx#L16-L31), setting maxWidth/Height to the values from the config, or figure out where to drop some global override, but that's starting to feel like just fighting the system

@yangshun
Copy link
Contributor

Thanks for reporting! I think we'll drop the max-width or make it configurable.

@slorber
Copy link
Collaborator

slorber commented Jun 17, 2022

On the Docusaurus side: I'll make sure that we can pass className/style to navbar/footer logos to override the defaults easily.


Changing this will impact the display of the footer logo of users websites. They may now need to add an explicit width, while it wasn't necessary before.

The max-width is useful in case the initial image is very large and the user does not provide an explicit width (which may be used by many in practice)

For example without width/max-width, this is what we could have by default: a very large OSS logo

CleanShot 2022-06-17 at 12 57 32@2x

What I suggest is to keep it, but use max-width: 500px instead.

That may be a good default for many logos

CleanShot 2022-06-17 at 13 02 09@2x


Note, another option could be something like max-width: min(500px, 90vw);

This permits to shrink the logo on small width so that it always stays in the viewport.

For example, if the user provides an explicit width=500 because they want a large icon on desktop, it will unfortunately not be shrinked on mobile, and it can lead to a horizontal scrollbar due to the logo overflowing.

(this is not a problem for our OSS logo here because we don't need to scale it up, it's already large by default, but some users may have small svgs and want to scale them up explicitly)

CleanShot 2022-06-17 at 13 06 39@2x

But min() is a new CSS property, apparently not supported everywhere yet, in particular in China (see also facebook/docusaurus#7617)


What I would try in the end:

.footer__logo {
  max-width: 500px;
}
@supports (max-width: min(0px)) {
  .footer__logo {
    max-width: min(500px, 90vw);
  }
}

Any opinion @yangshun ?

@Josh-Cena
Copy link
Contributor

Josh-Cena commented Jun 17, 2022

I don't want us to get too constrained by Chinese browser support. The situation in China is quite unique: we have a large number of low-end devices running outdated browsers, but as of 2022, the usage of websites is on a sharp decline. I don't think many people even use browsers that much anymore. (Most of the Internet services you access are native apps or microapps embedded in Wechat/Alipay/etc.) These figures are, I suppose (without any warrant), very distorted compared to their actual impact.

@Josh-Cena
Copy link
Contributor

If you ever take a look—11.19% of Chinese browsers are IE 😅 Another 11.91% for UC browser Android, which I don't think anyone actually uses to read Docusaurus sites. 5.79% for QQ browser, which I'm ambivalent about (I had been a QQ browser user like... ten years ago?)

@slorber
Copy link
Collaborator

slorber commented Jun 17, 2022

That wouldn't harm much to add that little @supports rule anyway 😄

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 a pull request may close this issue.

4 participants