feat(sitetitle): img component #2256

chappelo · 2024-08-25T15:38:13Z

Description

partially addresses SVG Logo during build doubles in dist/_astro folder #2161 by implementing Image for Logo, however, does not stop svg from being duplicated.

changeset-bot · 2024-08-25T15:38:16Z

⚠️ No Changeset found

Latest commit: 3568e05

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

netlify · 2024-08-25T15:38:30Z

✅ Deploy Preview for astro-starlight ready!

Name	Link
🔨 Latest commit	`3568e05`
🔍 Latest deploy log	https://app.netlify.com/sites/astro-starlight/deploys/66ddaba4d559c600081c90a7
😎 Deploy Preview	https://deploy-preview-2256--astro-starlight.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (no change from production) Accessibility: 100 (no change from production) Best Practices: 100 (no change from production) SEO: 92 (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.

astrobot-houston · 2024-08-25T15:38:40Z

Hello! Thank you for opening your first PR to Starlight! ✨

Here’s what will happen next:

Our GitHub bots will run to check your changes.
If they spot any issues you will see some error messages on this PR.
Don’t hesitate to ask any questions if you’re not sure what these mean!
In a few minutes, you’ll be able to see a preview of your changes on Netlify 🤩
One or more of our maintainers will take a look and may ask you to make changes.
We try to be responsive, but don’t worry if this takes a few days.

delucis

Thanks for tackling this @chappelo! I know you marked this as a draft, but I saw you were making good progress, so thought I’d leave some early feedback.

delucis · 2024-08-26T08:54:56Z

packages/starlight/components/SiteTitle.astro

 					height={logos.dark.height}
+					width={logos.dark.width}
+					alt={config.logo.alt}


The <Image> component loads the image lazily by default, so we should explicitly override that behaviour here given this is high priority content:

Suggested change

alt={config.logo.alt}

alt={config.logo.alt}

loading="eager"

Understood, thanks.

delucis · 2024-08-26T08:55:16Z

packages/starlight/components/SiteTitle.astro

 						class="dark:sl-hidden"
+						src={logos.light}
+						height={logos.light.height}
+						width={logos.light.width}
 						alt={config.logo.alt}


And then same here:

Suggested change

alt={config.logo.alt}

alt={config.logo.alt}

loading="eager"

chappelo · 2024-08-27T14:22:37Z

@delucis thanks for the feedback. would be eternally grateful if you could help clarify a couple things regarding the expected outcomes 🙇

<Image> component now used in <SiteTitle>
The Image asset is being referened twice.

Once in index.mdx:

hero:
  tagline: Congrats on setting up a new Starlight project!
  image:
    file: ../../assets/houston.webp

A second time in astro config:

logo: {
  src: './src/assets/houston.webp',
},

Upon running npm build, three images are processed:

A 400x400 Hero <Image>(houston.CZZyCf7p_tstkr)
A 800x800 Logo <Image>(houston.CZZyCf7p_Z1FoQLb)
unused: houston.CZZyCf7p

I should be aiming to remove the unused asset - which should have already been automatically removed ?
#2161 (comment)

Furthermore, if we want to reduce the output images down to one, I should further set the Logo's <Image> props too also be 400x400, so we don't build an 800x800 and 400x400 of the same image.

delucis · 2024-08-27T15:28:28Z

I should be aiming to remove the unused asset - which should have already been automatically removed ? #2161 (comment)

That issue is specific to SVGs — for other image formats we definitely still want two different outputs because they’ll be different sizes as you noticed. That said, I don’t know if Astro is smart enough not to duplicate even SVGs, but that would be the test case rather than the .webp Houston image.

chappelo · 2024-08-27T15:53:28Z

I should be aiming to remove the unused asset - which should have already been automatically removed ? #2161 (comment)

That issue is specific to SVGs — for other image formats we definitely still want two different outputs because they’ll be different sizes as you noticed. That said, I don’t know if Astro is smart enough not to duplicate even SVGs, but that would be the test case rather than the .webp Houston image.

ahh woops I got carried with .webp/png. Will investigate with SVG! Thank you.

chappelo · 2024-08-31T15:24:40Z

I should be aiming to remove the unused asset - which should have already been automatically removed ? #2161 (comment)

That issue is specific to SVGs — for other image formats we definitely still want two different outputs because they’ll be different sizes as you noticed. That said, I don’t know if Astro is smart enough not to duplicate even SVGs, but that would be the test case rather than the .webp Houston image.

Good evening, sorry for the wait.

I've done some digging, and as you already stated, the Image component changes alone aren't enough - the SVGs are still being duplicated. I've tried playing w/ config and some methods from /assets but to no avail.

I assume this should be addressed in core astro build process, not starlight(?). I'd happily do it, if you think that is the appropriate step.

note: I'll take this off draft for now.

delucis · 2024-09-03T09:49:13Z

Aha, good to know. My guess would be that because Astro hashes the options used with <Image> and getImage() when creating the image hash, those are taken into account even when the different width/height technically aren‘t impacting the output file, generating two hashes. Thanks for digging into it.

Bit fiddly to work around: either we manually switch to a regular <img> when we detect an SVG was passed to Starlight or like you say, look into fixing this in Astro itself.

chappelo · 2024-09-04T13:00:24Z

Aha, good to know. My guess would be that because Astro hashes the options used with <Image> and getImage() when creating the image hash, those are taken into account even when the different width/height technically aren‘t impacting the output file, generating two hashes. Thanks for digging into it.

Bit fiddly to work around: either we manually switch to a regular <img> when we detect an SVG was passed to Starlight or like you say, look into fixing this in Astro itself.

Cool thanks, will take another look this weekend!

chappelo · 2024-09-08T13:56:43Z

Aha, good to know. My guess would be that because Astro hashes the options used with <Image> and getImage() when creating the image hash, those are taken into account even when the different width/height technically aren‘t impacting the output file, generating two hashes. Thanks for digging into it.

Bit fiddly to work around: either we manually switch to a regular <img> when we detect an SVG was passed to Starlight or like you say, look into fixing this in Astro itself.

Good evening, SVG appears to no longer double with the conditional svg checks!

So I've implemented the checks in the relevant components which use the Image component: Hero, SiteTitle.

Had to play around w/ SiteTitle render logic abit to handle cases like these:

Any issues let me know :)

wip(sitetitle): img component

962ad4c

github-actions bot added the 🌟 core Changes to Starlight’s main package label Aug 25, 2024

delucis reviewed Aug 26, 2024

View reviewed changes

refactor(sitetitle): loading eager

a601adc

chappelo marked this pull request as ready for review August 31, 2024 15:26

chappelo changed the title ~~wip(sitetitle): img component~~ feat(sitetitle): img component Aug 31, 2024

chappelo added 2 commits September 8, 2024 22:11

refactor: handle svg

0a9b4cb

fix: dont render 2

3568e05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sitetitle): img component #2256

feat(sitetitle): img component #2256

chappelo commented Aug 25, 2024 •

edited

Loading

changeset-bot bot commented Aug 25, 2024 •

edited

Loading

netlify bot commented Aug 25, 2024 •

edited

Loading

astrobot-houston commented Aug 25, 2024

delucis left a comment

delucis Aug 26, 2024

chappelo Aug 27, 2024

delucis Aug 26, 2024

chappelo commented Aug 27, 2024 •

edited

Loading

delucis commented Aug 27, 2024

chappelo commented Aug 27, 2024 •

edited

Loading

chappelo commented Aug 31, 2024

delucis commented Sep 3, 2024

chappelo commented Sep 4, 2024

chappelo commented Sep 8, 2024

feat(sitetitle): img component #2256

Are you sure you want to change the base?

feat(sitetitle): img component #2256

Conversation

chappelo commented Aug 25, 2024 • edited Loading

Description

changeset-bot bot commented Aug 25, 2024 • edited Loading

⚠️ No Changeset found

netlify bot commented Aug 25, 2024 • edited Loading

✅ Deploy Preview for astro-starlight ready!

astrobot-houston commented Aug 25, 2024

delucis left a comment

Choose a reason for hiding this comment

delucis Aug 26, 2024

Choose a reason for hiding this comment

chappelo Aug 27, 2024

Choose a reason for hiding this comment

delucis Aug 26, 2024

Choose a reason for hiding this comment

chappelo commented Aug 27, 2024 • edited Loading

delucis commented Aug 27, 2024

chappelo commented Aug 27, 2024 • edited Loading

chappelo commented Aug 31, 2024

delucis commented Sep 3, 2024

chappelo commented Sep 4, 2024

chappelo commented Sep 8, 2024

chappelo commented Aug 25, 2024 •

edited

Loading

changeset-bot bot commented Aug 25, 2024 •

edited

Loading

netlify bot commented Aug 25, 2024 •

edited

Loading

chappelo commented Aug 27, 2024 •

edited

Loading

chappelo commented Aug 27, 2024 •

edited

Loading