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 <Steps> component #1564

Merged
merged 19 commits into from
Mar 1, 2024
Merged

Add <Steps> component #1564

merged 19 commits into from
Mar 1, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented Feb 27, 2024

Description

Copy link

changeset-bot bot commented Feb 27, 2024

🦋 Changeset detected

Latest commit: fd16093

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

Copy link

vercel bot commented Feb 27, 2024

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

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Mar 1, 2024 5:53pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Mar 1, 2024 5:53pm

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Feb 27, 2024
@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Feb 27, 2024

size-limit report 📦

Path Size
/index.html 6.09 KB (+5.79% 🔺)
/_astro/*.js 21.54 KB (0%)
/_astro/*.css 13.45 KB (0%)

@sarah11918
Copy link
Member

Woo! These are looking so good! I love the step numbers and the lines. The headings look really good, too!

Some gut reaction feedback on the visuals (haven't looked at the authoring experience yet):

  1. I first viewed this in dark mode and thought, "Oh, did we decide to NOT have the vertical lines?" I think the lighter grey for the lines, and honestly, maybe even the fill colour of the circles might be too subtle
    in dark mode?

image

  1. With component content: having the code snippet off to the right looks like it's a mistake? It feels off, I think?
  2. What if it's a regular code block within Markdown? I don't see an example of that, and that's one of our big use cases.

Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
@delucis
Copy link
Member Author

delucis commented Feb 27, 2024

  1. I first viewed this in dark mode and thought, "Oh, did we decide to NOT have the vertical lines?" I think the lighter grey for the lines, and honestly, maybe even the fill colour of the circles might be too subtle in dark mode?

I did go back and forth a bit, but the more intense hairline felt kind of noisy and distracting, especially in medium viewports where it kind of competes with the border of the sidebar in light mode:

sidebar border and steps guidelines running parallel

I’ll take another look though. Maybe this is a case where we need to tweak things in dark mode vs light mode.

  1. With component content: having the code snippet off to the right looks like it's a mistake? It feels off, I think?

That’s just an example, not how things automatically behave — I threw a <CardGrid> in to split those two things vertically kind of like this example. But if someone wants to do that, they’d be on the hook for it and would make it look as good/bad as they want unrelated to <Steps>.

  1. What if it's a regular code block within Markdown? I don't see an example of that, and that's one of our big use cases.

Do you mean like the example in the docs for the component?

image

@sarah11918
Copy link
Member

Do you mean like the example in the docs for the component?

Ah, yes I did! Sorry, I was only looking on the test page! That looks great!

@liruifengv
Copy link
Member

Wow! This looks so cool. We need it for our Astro docs tutorial.

@HiDeoo
Copy link
Member

HiDeoo commented Feb 28, 2024

Tested the new component in one project by replacing my existing <Steps> component with the new one as it uses a similar approach. Drop-in replacement worked perfectly 🎉 (altho it's only basic lists without headings so this does not cover all use cases).

Also played a bit with VoiceOver and as expected considering this is beautifully built around lists, it works perfectly.

Some more general questions:

  • Did you already check what the pa11y errors are?
  • Are some parts of the docs good candidates to showcase the new component? Do we want to do that? I thought the 3 getting started steps (Create a new project - Start the development server - Add content) could maybe be a good candidate to do that. Could also totally be follow-up PRs. Any thoughts?

@delucis
Copy link
Member Author

delucis commented Feb 28, 2024

  • Did you already check what the pa11y errors are?

Yes, started investigating last night but didn’t complete it yet. AFAICT it seems to be a false positive where it somehow thinks a background colour from the ::before/::after pseudo elements are overlapping with something? I didn’t yet fully understand but will investigate and fix somehow.

  • Are some parts of the docs good candidates to showcase the new component? Do we want to do that? I thought the 3 getting started steps (Create a new project - Start the development server - Add content) could maybe be a good candidate to do that. Could also totally be follow-up PRs. Any thoughts?

Yes! Locally I tried it already in the numbered steps on the “Customizing Starlight” guide and worked quite well. I’ll try your suggestion too.

My thought was that it might be best to do in a follow-up PR that could also apply the formatting change to other languages in one pass with i18nIgnore.

@delucis
Copy link
Member Author

delucis commented Feb 29, 2024

Wrapped up some investigation into the pa11y failures:

In some cases, when axe can’t determine the contrast ratio for certain elements, for example if the element has a background image or contains a pseudo-element, it will report the issue as “needs review”. The problem is that pa11y is currently reporting these review items as errors

(see pa11y/pa11y#633 (comment))

We have this exact issue: if I remove a background colour from the ::before used as the list marker, the tests pass, but adding it back causes them to fail.

This is tracked as an issue in pa11y/pa11y#623 and there’s a draft PR in pa11y/pa11y#685 but no fix yet. (Also would be fixed in pa11y@7, but pa11y-ci is still on pa11y@6, so lots of stacks of maintenance needed there — shame that seems to be quite slow, wonder if there’s anything we can do to help?)

Given this, I think we’re out of options apart from disabling colour contrast checks in pa11y-ci for now. (We should bring them back once things update hopefully.) Colour contrast is luckily pretty easy to test manually — and even spot visually for the most part — so I think we have a fairly good handle on that and shouldn’t let anything slip by without the automated check 🤞 Made that change in 4214d5b and tests should now pass.

@HiDeoo
Copy link
Member

HiDeoo commented Mar 1, 2024

Made that change in 4214d5b and tests should now pass.

This makes sense and I think we should indeed be fine for the time being. 👍

I also want at some point to take the time to test kayle which I found out when the PR to migrate the CI part of pa11y-ci into pa11y was cancelled (they also seem to work on a Rust/WASM runner with the A11yWatch org so maybe good to keep an eye on that too).

Simplify styles by applying `sl-steps` class directly to `<ol>` rather than relying on Astro scoping and a wrapper `<div>`
@delucis
Copy link
Member Author

delucis commented Mar 1, 2024

I also want at some point to take the time to test kayle

Interesting! Hadn’t seen that — seems early still with one developer although they are building a business of it looks like, so maybe there’ll be more investment in the tools if they are successful.


OK, one last update from me this morning: I decided to remove the wrapper <div> this component was outputting. We only had it there to get a scoped class name for styling, and I decided we could get by without that — just making the sl-steps class global. Simplifies things a bit and removes unnecessary wrapper elements. (Also as a side effect technically means people can slap the sl-steps class on anything they wanted I guess.)


@sarah11918 Re: your concern about the dark theme guide line — I did end up bumping up the contrast a touch on that, so hopefully should address that!

Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢 🚢 🚢 🚢

@delucis delucis merged commit d880065 into main Mar 1, 2024
9 checks passed
@delucis delucis deleted the dx-1012/steps branch March 1, 2024 17:55
@astrobot-houston astrobot-houston mentioned this pull request Mar 1, 2024
HiDeoo added a commit to HiDeoo/starlight that referenced this pull request Mar 2, 2024
* main:
  [ci] format
  [ci] release (withastro#1574)
  Add `<Steps>` component (withastro#1564)
  Add `<FileTree>` component (withastro#1308)
  Add icon support to the `<TabItem>` component (withastro#1568)
  [ci] format
  docs: add Flojoy to showcase (withastro#1571)
  i18n(es): update `components` (withastro#1547)
  i18n(pt-PT): add "manual-setup" page (withastro#1570)
  i18n(zh-cn): Update pages.mdx (withastro#1565)
  Updates internal github actions to the latest versions (withastro#1569)
  [ci] format
  i18n(it): Update pages.mdx & plugins.mdx (withastro#1567)
  [ci] format
  i18n(pt-PT): add "environmental-impact" page (withastro#1561)
  [ci] format
  i18n(zh-cn): Update plugins.mdx (withastro#1566)
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.

6 participants