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

feat(Testimonials): add testimonials to website #1104

Merged
merged 24 commits into from
Jan 11, 2024
Merged

feat(Testimonials): add testimonials to website #1104

merged 24 commits into from
Jan 11, 2024

Conversation

ElianCodes
Copy link
Member

@ElianCodes ElianCodes commented Nov 21, 2023

Description

Adds Testimonials to starlight.astro.build.

For now, this is an initial draft of adding testimonials to the Starlight website, using components. I created a draft PR for initial feedback on both design and initial API design.

Screenshot 2023-11-21 at 17 22 14

(lot's of inspiration taken from Starlight's inner Card component)

Goal

I would like it to be added to Starlight Core in a future release. I'm aiming to make everything compatible with Starlight and it's styling.

Things to note

  • We probably want the testimonials to be <ul> and <li> elements
  • How do we approach images here?
  • Do we want to link to used tweets, posts, profiles, whatever

@ElianCodes ElianCodes added the 📚 docs Documentation website changes label Nov 21, 2023
@ElianCodes ElianCodes self-assigned this Nov 21, 2023
Copy link

changeset-bot bot commented Nov 21, 2023

⚠️ No Changeset found

Latest commit: 1bbf571

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

Copy link

vercel bot commented Nov 21, 2023

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

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Jan 11, 2024 8:00pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Visit Preview Jan 11, 2024 8:00pm

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Nov 21, 2023

size-limit report 📦

Path Size
/index.html 5.21 KB (0%)
/_astro/*.js 21.42 KB (0%)
/_astro/*.css 13.05 KB (0%)

@HiDeoo
Copy link
Member

HiDeoo commented Nov 22, 2023

I did not look at the code at all but wanted to give my opinion on the feature/design itself.

Do we want to link to used tweets, posts, profiles, whatever

I know that personally, when I look at testimonials, I always try to find a link to see the relevant post or tweet which would usually contains more infos than a cherry picked quote.

I also think it would be nice to have a date, sometimes a quote is not relevant anymore if many major changes happened since then (or if the quote is just too old) and I personally think having a date add more credibility to the quote. Any thoughts on that?

@ElianCodes
Copy link
Member Author

Thanks @HiDeoo! Those are very good ideas!

yesterday during the community call, I saw that Knip Docs has implemented Testimonials by themselves with the Card components

@kevinzunigacuellar
Copy link
Sponsor Member

I usually not see this on testimonials but I was thinking it would be a nice to have a source property (e.g twitter (x), mastodon, instagram?) or maybe we could extract the source from the url link? Open to suggestions 💡 .

@vasfvitor
Copy link
Contributor

vasfvitor commented Nov 30, 2023

I'm not a UI guy, but I had this in mind so I did a quick gimp edit, now that I see it looks odd to me.

Besides this, I feel there should be more spacing above testimonials

I like avatar like images for testimonials, in this case the cards feels empty, maybe could fit 3 per line?

Another idea: each card would split in the middle, where left side would be information about the person/company (picture, social handle, link to testimonial) and left side the quotes in a quote block

Co-authored-by: Genteure <Genteure@users.noreply.github.com>
Co-authored-by: Reuben Tier <TheOtterlord@users.noreply.github.com>
Co-authored-by: Atharva Pise <atharvapise19@gmail.com>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: HiDeoo <HiDeoo@users.noreply.github.com>
@HiDeoo
Copy link
Member

HiDeoo commented Nov 30, 2023

Not a review but just a small update as a follow-up of the T&D session from today:

  • If we want proper LTR/RTL support, I did some research regarding blockquotes in RTL and it looks like the decoration should be flipped which match the current design for the border but not the text:
image
  • We may want to support an optional lang attribute too according to this reference.

  • We may want to use the cite attribute to link to the source of the quote if we have one.

@ElianCodes ElianCodes marked this pull request as ready for review November 30, 2023 16:29
Copy link
Member

@at-the-vr at-the-vr left a comment

Choose a reason for hiding this comment

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

There are plans to move Testimonial into starlight's core right? Can't think of pending tasks besides that and Hideo's RTL/Citation update

@ElianCodes
Copy link
Member Author

Yes! I'll build in the cite function sometime today!

The plan would be to first add it to our Starlight website, before finetuning and building it into Starlight core.

@github-actions github-actions bot added the i18n Anything to do with internationalization & translation efforts label Dec 4, 2023
@HiDeoo
Copy link
Member

HiDeoo commented Dec 4, 2023

Did some experiments with RTL and I think this only requires a few changes to be correct.

First, in docs/src/components/testimonial-grid.astro, the .testimonial-grid class left padding should be updated:

	.testimonial-grid {
		list-style: none;
		display: grid;
		gap: 1rem;
-		padding-left: 0;
+		padding-inline-start: 0;
	}

This avoids a padding issue in RTL:

SCR-20231204-quuf

Then, to fix the text alignment issue I mentioned earlier, I think in docs/src/components/testimonial.astro we can add a text-align property to the .testimonial-body class:

	.testimonial-body {
		margin-bottom: 0 !important;
		display: flex;
		flex-direction: column;
		gap: 1rem;
		font-size: clamp(var(--sl-text-sm), calc(0.5rem + 1vw), var(--sl-text-body));
+		text-align: start;
	}

start: The same as left if direction is left-to-right and right if direction is right-to-left.

Altho, the date of a testimonial should probably still be aligned based on the page direction and not the testimonial direction.

<div class="testimonial-body" dir={dir ?? pageDir}>
  {content ? <blockquote cite={link ? link.toString() : ''} set:html={content} /> : null}
  <slot />
-  {date ? <span>posted on {date?.toLocaleDateString()}</span> : null}
</div>
+{date ? <span class="testimonial-date">posted on {date?.toLocaleDateString()}</span> : null}

which requires adding the following CSS to re-introduce the top between the testimonial body and the date:

+.testimonial-date {
+  display: block;
+  margin-top: 1rem;
+}

Here are how it looks with the changes (which I think is the expected result):

SCR-20231204-qwsu SCR-20231204-qxir

@delucis
Copy link
Member

delucis commented Jan 11, 2024

Update on the work here as I’ve picked it up from @ElianCodes!

  • I’ve updated index.mdx to now contain more content from different people — more Starlight love 💖

  • I’ve simplified the grid and testimonial components quite a bit:

    • Content is now always slotted in
    • There are only three props — name, handle, and cite — and all are required.
    • Avatars are stored in the repo in docs/src/assets/testimonials/ and referred to via each person’s handle.
  • I removed the explicit language/writing direction props. The components will still respond compatibly in an RTL layout but my thinking is that the content should be translated for the current language, so we don’t need the component to support, e.g. Arabic text in an English page or vice-versa.

  • I brought back a heading — using @doodlemarks’s suggested “What people are saying” copy as I found without it, the transition to these from the preceding cards felt odd, especially on smaller viewports where you have less context from other quotes to help understand you’re looking at a collection of quotes.

  • I took some liberties to scale down the component sizing on smaller viewports — most stuff is responsive to a single responsive type scale.

Screenshots

Desktop Mobile
six testimonials showing in two columns on a large desktop viewport two testimonials showing on a small mobile viewport

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

These look great to me, on desktop and on phone! I think the ordering is sensible. Approve!

@kevinzunigacuellar
Copy link
Sponsor Member

Looks good on iPhone (miss my fold). One comment, in mobile it's easy to confuse comments from other people because the space between comments and comment/authors is very similar.

@doodlemarks
Copy link
Contributor

Looks great @ElianCodes!

No design feedback from me. Only thing I would advise is limiting this list to less (6?) quotes is more effective IMO. Having such a large wall of text and some of the quotes are better than others.

@sarah11918
Copy link
Member

sarah11918 commented Jan 11, 2024

I do agree with Kevin's comment. On my phone (super early this morning, mind you!) I did have to scroll up to the top to figure out whether the name came before or after the comment. (Wasn't sure how much of that was me at 4am, nor how much people scrolling on a phone super care. But agreed that when you've scrolled down that far on mobile, it's not totally obvious.

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.

Semi-self-approval!

Thanks for kicking this off @ElianCodes and thanks for everyone’s feedback along the way 🙌

@delucis delucis merged commit 238e361 into main Jan 11, 2024
9 checks passed
@delucis delucis deleted the add-testimonial branch January 11, 2024 20:06
@HiDeoo
Copy link
Member

HiDeoo commented Jan 13, 2024

  • I removed the explicit language/writing direction props. The components will still respond compatibly in an RTL layout but my thinking is that the content should be translated for the current language, so we don’t need the component to support, e.g. Arabic text in an English page or vice-versa.

Oh, this is great. I was under the (wrong) impression that this should not be translated content so this definitely simplifies things a lot 👍 Thanks for clarifying and cleaning up.

HiDeoo added a commit to HiDeoo/starlight that referenced this pull request Jan 15, 2024
* main: (55 commits)
  [ci] format
  i18n(es): Update `index` (withastro#1360)
  [ci] format
  i18n(fr): Update index (withastro#1367)
  i18n(es): remove extra section (withastro#1370)
  i18n(ko-KR): update `index.mdx` (withastro#1363)
  [ci] format
  i18n(zh-cn): Update index.mdx (withastro#1361)
  docs(showcase): add OpenSaaS.sh (withastro#1359)
  feat(Testimonials): add testimonials to website (withastro#1104)
  [ci] format
  [ci] release (withastro#1332)
  fix: autogenerated sidebar alphabetical sort (withastro#1298)
  Avoid sidebar scrollbar hiding behind navbar (withastro#1353)
  Use spawnSync instead of execaSync in `git.ts` (withastro#1347)
  [ci] format
  [i18nIgnore] Add src alias (withastro#1322)
  Italian translation for search.devWarning (withastro#1351)
  [ci] format
  i18n(pt-BR): Add translation for `guides/sidebar` (withastro#1346)
  ...
HiDeoo added a commit to HiDeoo/starlight that referenced this pull request Jan 15, 2024
* main: (69 commits)
  [i18nIgnore] docs: `pnpm install` → `pnpm add` (withastro#1324)
  [ci] format
  i18n(zh-cn): Update frontmatter.mdx (withastro#1362)
  [ci] format
  i18n(es): Update `index` (withastro#1360)
  [ci] format
  i18n(fr): Update index (withastro#1367)
  i18n(es): remove extra section (withastro#1370)
  i18n(ko-KR): update `index.mdx` (withastro#1363)
  [ci] format
  i18n(zh-cn): Update index.mdx (withastro#1361)
  docs(showcase): add OpenSaaS.sh (withastro#1359)
  feat(Testimonials): add testimonials to website (withastro#1104)
  [ci] format
  [ci] release (withastro#1332)
  fix: autogenerated sidebar alphabetical sort (withastro#1298)
  Avoid sidebar scrollbar hiding behind navbar (withastro#1353)
  Use spawnSync instead of execaSync in `git.ts` (withastro#1347)
  [ci] format
  [i18nIgnore] Add src alias (withastro#1322)
  ...
HiDeoo added a commit to HiDeoo/starlight that referenced this pull request Jan 15, 2024
* main: (62 commits)
  [i18nIgnore] docs: `pnpm install` → `pnpm add` (withastro#1324)
  [ci] format
  i18n(zh-cn): Update frontmatter.mdx (withastro#1362)
  [ci] format
  i18n(es): Update `index` (withastro#1360)
  [ci] format
  i18n(fr): Update index (withastro#1367)
  i18n(es): remove extra section (withastro#1370)
  i18n(ko-KR): update `index.mdx` (withastro#1363)
  [ci] format
  i18n(zh-cn): Update index.mdx (withastro#1361)
  docs(showcase): add OpenSaaS.sh (withastro#1359)
  feat(Testimonials): add testimonials to website (withastro#1104)
  [ci] format
  [ci] release (withastro#1332)
  fix: autogenerated sidebar alphabetical sort (withastro#1298)
  Avoid sidebar scrollbar hiding behind navbar (withastro#1353)
  Use spawnSync instead of execaSync in `git.ts` (withastro#1347)
  [ci] format
  [i18nIgnore] Add src alias (withastro#1322)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 docs Documentation website changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants