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

Add LinkCard Component #397

Merged
merged 29 commits into from
Aug 10, 2023
Merged

Add LinkCard Component #397

merged 29 commits into from
Aug 10, 2023

Conversation

lorenzolewis
Copy link
Contributor

@lorenzolewis lorenzolewis commented Jul 25, 2023

What kind of changes does this PR include?

  • Changes to Starlight code

Description

Opening as a draft for now to get some feedback. Relevant discussion on Discord: https://discord.com/channels/830184174198718474/1133135977628577813

  • Create a LinkCard component for a nicely styled clickable card
  • Update .content CSS to allow styling for components explicitly marked as .content (this was done because the LinkCard component is an a element and top-margin was not being applied. Open to a different way on this if some better suggestions)

Signed-off-by: Lorenzo Lewis <lorenzo@crabnebula.dev>
Signed-off-by: Lorenzo Lewis <lorenzo@crabnebula.dev>
Signed-off-by: Lorenzo Lewis <lorenzo@crabnebula.dev>
Signed-off-by: Lorenzo Lewis <lorenzo@crabnebula.dev>
@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2023

🦋 Changeset detected

Latest commit: 746bf99

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

@netlify
Copy link

netlify bot commented Jul 25, 2023

Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit 746bf99
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/64d35183e6c5ba0008374184
😎 Deploy Preview https://deploy-preview-397--astro-starlight.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.

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Jul 25, 2023
@HiDeoo
Copy link
Member

HiDeoo commented Jul 25, 2023

Neat, this would be a great addition, I needed something like this recently. Some quick thoughts/questions:

  • I wonder if I wouldn't expect the LinkCard props to extends HTMLAttributes<'a'> so a user can pass in any valid <a> props. This would mean that link could be removed as the user could just pass in href instead.
  • title may be a bit confusing as it's the card title and not the link title which is a valid link attribute. If we decide to accept any <a> props, then title would be a valid prop for the link and we may need another prop for the card title.

@delucis
Copy link
Member

delucis commented Jul 25, 2023

Thanks for this! Going to wait for some feedback from @doodlemarks before looking too much into the implementation — looped Mark in in the Discord thread: https://discord.com/channels/830184174198718474/1133135977628577813

Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
@lorenzolewis
Copy link
Contributor Author

Thanks @HiDeoo , I put that in. Always a pleasure learning from you 💜

Updated design based on discussion in the Discord thread

@HiDeoo
Copy link
Member

HiDeoo commented Jul 25, 2023

Well, this was mostly my own thoughts / questions, maybe that's just bad ideas ^^ We will see what the team thinks about it.

@lorenzolewis
Copy link
Contributor Author

Well, this was mostly my own thoughts / questions, maybe that's just bad ideas ^^ We will see what the team thinks about it.

No worries, can always revert it if someone says otherwise

Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
Signed-off-by: Lorenzo Lewis <lorenzo_lewis@icloud.com>
@lorenzolewis lorenzolewis marked this pull request as ready for review July 26, 2023 11:44
@lorenzolewis
Copy link
Contributor Author

Alright, this one is ready for review. These are the things that I know of that didn't get included in this one:

  • 3 column layout for CardGrid: I'm not 100% sure how this could be accomplished without exposing a new prop in CardGrid or making a different component. Maybe a separate PR for this one if it's wanted?
  • Image for leading edge: Would still like to add this to fill the use case for the deploy page in the mockup, but would prefer getting this merged in first and then tackling that one separately.

The code block on the documentation page felt a bit long to me, so if anyone else agrees I can refactor that to reduce the number of cards.

@lorenzolewis lorenzolewis changed the title draft: Link Card Component Add Link Card Component Jul 26, 2023
@lorenzolewis lorenzolewis changed the title Add Link Card Component Add LinkCard Component Jul 26, 2023
@delucis
Copy link
Member

delucis commented Aug 8, 2023

@lorenzolewis Thank for your patience on this once! Was hoping to get a release out with this today so commandeered your work, only to run out of time in the day 😅

So apologies for committing and not reviewing! Let me at least leave some notes on what I changed:

  • Pretty sure we’ll need that RTL support elsewhere in the future, so moved it to a utility class. Also made it use a matrix() transform to mirror the element across the vertical axis instead of rotating 180°. That’s only subtly different and doesn’t impact our symmetrical arrow icon in this case, but could be relevant for some icons, think 👉 vs 👈 for example.

  • Made some small tweaks to the colors used to more closely match our existing hover styles. Super subtle changes as what you had was already great.

  • Refactored the component HTML and CSS a bit to simplify it. A summary:

    • Removed the nested <span>s to use set:html directly on the parent <span>
    • Wrapped each link in a <div> — this solves it being treated correctly with spacing in the document flow.
    • Removed some CSS that wasn’t having an effect anymore.
  • Streamlined the docs as you suggested above: smaller code example showing with/without description and inside/outside <CardGrid>. Also simplified the text a bit.

Given I basically invented the rush in my head and we actually do have time to discuss, I do have a question: how important do we think it is for someone to have the ability to set title on the resulting links? I don’t love the API difference of <Card title="Starlight" /> and <LinkCard heading="Starlight" /> and I also can’t imagine a valid use case for the regular title attribute with this component. Output like the following would be odd right?

<a title="Titled link" href="/">Link text</a>

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.

If people agree we can stick with title, we can make these changes:

packages/starlight/user-components/LinkCard.astro Outdated Show resolved Hide resolved
packages/starlight/user-components/LinkCard.astro Outdated Show resolved Hide resolved
packages/starlight/user-components/LinkCard.astro Outdated Show resolved Hide resolved
docs/src/content/docs/guides/components.mdx Outdated Show resolved Hide resolved
docs/src/content/docs/guides/components.mdx Outdated Show resolved Hide resolved
@HiDeoo
Copy link
Member

HiDeoo commented Aug 8, 2023

If people agree we can stick with title, we can make these changes:

I've seen this technique used a lot but it's not super recommended (even in this link) so personally I think it should be fine.

@delucis
Copy link
Member

delucis commented Aug 8, 2023

Great link — thank you! Yeah, I think that case should be covered by being able to add a description. The other generic use case I can imagine is things like <a title="Page about stuff">Read more</a> but that too seems like a bad idea to me and not really relevant to this component.

I’m generally in favour of “the most accessible text is visible and accessible to all technologies” an a component restricting usage a little bit to encourage correct output also seems part of the motivation for making components, so leaning towards API consistency here.

@lorenzolewis
Copy link
Contributor Author

Thanks for the changes @delucis (and you're always welcome to commandeer my PRs anytime, I just enjoy learning through them 🥳 )

For my personal use case using title instead of heading semantically makes a lot more sense to me (and was what I originally had before the amazing @HiDeoo helped me realise title is actually a valid attribute). So if we're okay with this being a (most likely never hit) edge case then I'd prefer title over heading.

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.

OK, let’s do it! Made that change and will get this out in the next release.

Thanks again for the great work here @lorenzolewis!

@delucis delucis added the 🌟 minor Change that triggers a minor release label Aug 9, 2023
@lorenzolewis
Copy link
Contributor Author

Woot! Then I can work on my next evil plan for a Table of Contents component 😈 😆 (will open an issue to discuss first <3 )

@delucis delucis merged commit 73eb5e6 into withastro:main Aug 10, 2023
10 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Aug 9, 2023
@lorenzolewis lorenzolewis deleted the feat/link-card branch August 10, 2023 10:00
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 📚 docs Documentation website changes 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants