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

Enlarge header icons #2661

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

Conversation

illright
Copy link
Contributor

@illright illright commented Dec 7, 2024

Description

Related to #1701, but not explicitly proposed there. The original proposal suggests to increase the space between the social icons, but I believe the icons themselves should be larger too.

This PR changes the appearance of social icons and the icons in language/theme selects to be larger. It also increases the touch area of the social icons to align it with the touch area of selects (~44.5px on desktop, ~48px on mobile). Finally, it marginally increases the space between the icon and the label, just something that's been bugging me visually a little bit.

Screenshots

Desktop (light)

BeforeAfter
Scherm­afbeelding 2024-12-07 om 19 22 40 Scherm­afbeelding 2024-12-07 om 19 23 17

Desktop (dark)

BeforeAfter
Scherm­afbeelding 2024-12-07 om 19 22 46 Scherm­afbeelding 2024-12-07 om 19 23 23

Mobile (light)

BeforeAfter
Scherm­afbeelding 2024-12-07 om 19 21 17 Scherm­afbeelding 2024-12-07 om 19 16 59

Mobile (dark)

BeforeAfter
Scherm­afbeelding 2024-12-07 om 19 21 22 Scherm­afbeelding 2024-12-07 om 19 17 12

Copy link

changeset-bot bot commented Dec 7, 2024

🦋 Changeset detected

Latest commit: 928686f

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 Minor

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

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Dec 7, 2024
Copy link

netlify bot commented Dec 7, 2024

Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit 928686f
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/675492ec2c3bd80008dc4a84
😎 Deploy Preview https://deploy-preview-2661--astro-starlight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

Copy link
Contributor

@SnowDingo SnowDingo left a comment

Choose a reason for hiding this comment

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

Thank you @illright for your contribution!

I took a look at the changes you've made and it seems like everything is working properly and as it should. I think your change made the Starlight docs more accessible.
So I will approve the PR.

Snowdingo

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.

Thanks again for the PR @illright! Sharing some quick thoughts:

This has some definite positive sides (mainly of interest to me is making the clickable size a bit larger for accessibility), but I’m not so confident on the increased size and spacing overall.

A couple of points:

  • Because the icon size increase also requires the text to increase in the theme and language pickers, it has reduced available space inside those. This is probably not an obvious detail, but one issue we have with these menus is how they accommodate different strings. For example, in English the theme picker uses a set of relatively similar length strings (light/dark/auto) but in other languages this can vary quite a bit, e.g. Spanish which ranges from the short “Claro” to the long “Automático”.

    Currently, these menus are of a fixed width which is also why they contain in some cases (e.g. English) extra whitespace between the selected value and the caret on the right. The fixed width has the advantage of visual stability when selected values change (UI to the left of the menu does not need to shift horizontally to accommodate longer values) as well as predictability as you don’t need to click through all values to see if your layout works for all the different possibilities. But it has the drawback that we need to select a single “compromise” value for the menu width that will work for as many cases as possible without becoming too stretched for shorter values.

    This is not a new constraint in this PR, just noting that if scaling up the icons and text, we would also need to scale up the full field including whitespace to match.

  • I think the larger social links more or less work in the case of Starlight’s own docs (although personally I find the spacing a little wide), but I suspect the size + spacing increase could look less nice for sites using more links. It’s fairly common to have 4 or 5 of these, which starts to take up a lot of space at this size.

Here’s an example that maybe demonstrates the two issues above:

Before image
After image

Given this, I’m not sure what would be best here. Perhaps scoping things just to the clickable size of the icons with minimal visual changes would be an option? Curious to hear if this extra information triggers any new thoughts for you!

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.

3 participants