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: prevent text from overflowing prev/next links #814

Conversation

julien-deramond
Copy link
Contributor

@julien-deramond julien-deramond commented Oct 6, 2023

What kind of changes does this PR include?

  • Changes to Starlight code

Before the final merge if/when approved

  • Remove 2cd86e9 which gathers the extra long texts added for testing purposes
    • ⚠️ This commit breaks pa11y-ci job temporarily

Description

This PR is a follow-up of #756 where we decided to extract this fix for prev/next links in a separate PR. We agreed at the time that the suggested fix wasn't good enough because relying on media queries and values that don't exist already as media queries in Starlight (see discussion from #756 (comment)).

A possible simple fix would have been to use media containers because what we really want is a specific rendering based on the size of the container but the browsers' support is not yet good enough.

So I had to rely on some advanced features of display: grid often used to create complex layouts.

The following code already existed and worked well for the rendering of each pagination item.

a {
  flex-basis: calc(50% - 0.5rem);
  flex-grow: 1;
}

Since we are playing with auto layouts and sizes, the icon size can sometimes be reduced (or the icon can "disappear"), so the following allows us to keep the same size for the icon:

svg {
  flex-shrink: 0
}

The most complex part is definitely the following:

.pagination-links {
  display: grid;
  grid-template-columns: repeat(auto-fit, minmax(200px, 1fr));
  grid-gap: 1rem;
}

The grid-template-columns property defines the columns of the grid. The repeat(auto-fit, minmax(200px, 1fr)) value means that the grid will automatically create as many columns as it can fit in the available space, with a minimum width of 200px and a maximum width of 1fr (which means the columns will take up equal space).
If I say it differently, the pagination item will take the whole space until there's enough space to render two 200px pagination items on the same row.

The grid-gap property sets the gap between the columns.

Tested environments

In terms of browsers' support, it seems to be safe to use:

So I tested only the oldest combinations of browsers/versions listed in our compatibility browsers list):

  • Firefox 118.0.1 / macOS 14.1 (my env by default)
  • Chrome 88 / Windows 8
  • Edge 88 / Windows 8
  • iPhone 12 Pro Max / Safari 16
    • ⚠️ I don't have any 15.4 env to test on BrowserStack, before 15.4, the layout is completely broken so untestable
  • Firefox 98 / macOS 12.1

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2023

🦋 Changeset detected

Latest commit: f3922fe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Oct 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Oct 6, 2023 7:23pm

@netlify
Copy link

netlify bot commented Oct 6, 2023

👷 Deploy Preview for astro-starlight processing.

Name Link
🔨 Latest commit f3922fe
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/65380220fd4b000007157d26

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Oct 6, 2023
@julien-deramond julien-deramond force-pushed the main-jd-prevent-text-overflow-for-prev-next-link branch from b29541c to 2cd86e9 Compare October 6, 2023 19:22
@julien-deramond julien-deramond force-pushed the main-jd-prevent-text-overflow-for-prev-next-link branch from a3a748a to 4961f7e Compare October 12, 2023 04:29
@julien-deramond julien-deramond marked this pull request as ready for review October 12, 2023 04:39
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this solution! Thank you for cracking it @julien-deramond 💖

Left a couple of small notes, but I think this is 100% the right solution.

packages/starlight/components/Pagination.astro Outdated Show resolved Hide resolved
packages/starlight/components/Pagination.astro Outdated Show resolved Hide resolved
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome — the preview looks good to me! For future reference if anyone wants to see it to test out, it should be here for posterity: https://653800362ca82700088bafb1--astro-starlight.netlify.app/getting-started/

@julien-deramond I think it should be safe to go ahead and remove the test files 🥳

@julien-deramond julien-deramond force-pushed the main-jd-prevent-text-overflow-for-prev-next-link branch from 69d6f17 to f3922fe Compare October 24, 2023 17:42
@github-actions github-actions bot removed the 📚 docs Documentation website changes label Oct 24, 2023
@julien-deramond
Copy link
Contributor Author

julien-deramond commented Oct 24, 2023

  • 🟢 Fork synced with the main branch
  • 🟢 Branch rebased onto main branch, squashed, and is now without the extra commit
  • 🤞 Waiting for the deployment to check the rendering, but should be good to go
  • 🟢 Everything seems to be OK.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! Thanks again for your persistence figuring this out. Really appreciate it 🤩

@delucis delucis merged commit 1e517d9 into withastro:main Oct 24, 2023
9 checks passed
@julien-deramond julien-deramond deleted the main-jd-prevent-text-overflow-for-prev-next-link branch October 24, 2023 17:50
@astrobot-houston astrobot-houston mentioned this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants