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

Fix <DocCard> height in theme styles #10849

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hichemfantar
Copy link
Contributor

@hichemfantar hichemfantar commented Jan 17, 2025

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Ensure that the <DocCard> component maintains full height in its styles for better layout consistency.

Test Plan

before
image

after
image

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 17, 2025
@hichemfantar hichemfantar marked this pull request as ready for review January 17, 2025 08:48
Copy link

netlify bot commented Jan 17, 2025

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit bd5e723
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/678a18fbb023d00008436ecd
😎 Deploy Preview https://deploy-preview-10849--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@slorber slorber added pr: bug fix This PR fixes a bug in a past release. Argos Add this label to run UI visual regression tests. See argos.yml GH action. labels Jan 17, 2025
Copy link

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO Report
/ 🟠 71 🟢 98 🟢 96 🟢 100 Report
/docs/installation 🟠 52 🟢 97 🟢 100 🟢 100 Report
/docs/category/getting-started 🟠 73 🟢 100 🟢 100 🟠 86 Report
/blog 🟠 68 🟢 96 🟢 100 🟠 86 Report
/blog/preparing-your-site-for-docusaurus-v3 🔴 46 🟢 92 🟢 100 🟢 100 Report
/blog/tags/release 🟠 63 🟢 96 🟢 100 🟠 86 Report
/blog/tags 🟠 73 🟢 100 🟢 100 🟠 86 Report

Copy link

argos-ci bot commented Jan 17, 2025

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Jan 17, 2025, 8:56 AM

@slorber
Copy link
Collaborator

slorber commented Jan 17, 2025

Hmm, our visual regressions test shows 0 change.

I can't really see a difference between these 2 pages:

Is there anywhere in the site (including our dogfood /test section) I can review if it works as intended, and where prod shows it as broken?

@hichemfantar
Copy link
Contributor Author

bug is brought to light here
https://deploy-preview-10847--docusaurus-2.netlify.app/tests/docs/

@slorber
Copy link
Collaborator

slorber commented Jan 17, 2025

I do see the difference now:

There are multiple ways to achieve this, but height: 100% looks good enough 👍

@hichemfantar
Copy link
Contributor Author

can you share the other possible ways to fix this?

@hichemfantar
Copy link
Contributor Author

also any idea why visual testing didn't detect the change?

@slorber
Copy link
Collaborator

slorber commented Jan 17, 2025

Hmmm actually I'm not super fan of this solution: this makes the <DocCard> component assume it's only used in a <DocCardList> context and that it should always takes height: 100%.

In the future we may add this component to our "theme design system" so that users can use it as a standalone component. Forcing a 100% height by default is not ideal.

I'd prefer if <DocCardList> takes the responsibility of deciding that <DocCard> takes full height.

Alternative solutions probably involve using things like display flex / alignItems stretch / flex 1 0


also any idea why visual testing didn't detect the change?

Probably a little bug in my "screenshot exclusions" in screenshot.spec.ts: we never screenshot that page.

Edit: Oh I see, the index doc has

unlisted: true # Makes the navbar link disappear in prod

This effectively add a noindex tag which makes it disappear from sitemap.xml which makes it disappear from the site screenshots.

Unfortunately, this is on purpose since we want the unlisted item to automatically disappear from the navbar. This is not a big deal if we don't screenshot that page, unfortunate that in this specific PR we would have liked a screenshot 😅


But this screenshot is a bit surprising 🤔

https://app.argos-ci.com/meta-open-source/docusaurus/builds/1105/131471586

https://deploy-preview-10849--docusaurus-2.netlify.app/tests/docs/category/tests

Edit: we take screenshots in DEV mode and an extra "draft doc" is kept, this creates an offset in the list and makes this bug appear on the "External link test" card:

CleanShot 2025-01-17 at 11 07 28@2x

Then we have this CSS rule that messes things up 😅

@media (min-width: 997px) {
    .list_ZO3j article:nth-last-child(-n+2) {
        margin-bottom: 0px !important;
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Argos Add this label to run UI visual regression tests. See argos.yml GH action. CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants