Skip to content

Tutorial Styling Improvements#2852

Merged
yanthomasdev merged 9 commits into
mainfrom
yan/tutorial-improvements
Mar 16, 2023
Merged

Tutorial Styling Improvements#2852
yanthomasdev merged 9 commits into
mainfrom
yan/tutorial-improvements

Conversation

@yanthomasdev
Copy link
Copy Markdown
Member

What kind of changes does this PR include?

  • Changes to the docs site code

Description

A few changes to get our tutorial ready for translations and to improve accessibility.

  • Added RTL styles to <Box/> and <Checklist/> components:
Before After
image image
  • Increased line-height for lists with <kbd/> elements
Before After
image image
  • Improved Tutorial Tracker accessibility with better steps color contrast (now passing WCAG AAA) and added the missing focus style for the tab:
Before After
image image

@yanthomasdev yanthomasdev added site improvement Some thing that improves the website functionality - ask @delucis for help! a11y Anything to do with improving accessibility tutorial Content/UI for the Build a Blog Tutorial labels Mar 14, 2023
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 14, 2023

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit 9c759ad
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/64130ae259d7df0009eb91c1
😎 Deploy Preview https://deploy-preview-2852--astro-docs-2.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 settings.

@sarah11918
Copy link
Copy Markdown
Member

Thanks for this, @yan-thomas! I'll ask @delucis to approve this, then we can move onto the i18nReady pages!

Copy link
Copy Markdown
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 for tackling this, Yan! I’ve left some suggestions as I think we’ll still want to polish this a bit more.

Leaving a note for myself that I also want to make sure i18n progress is synced across languages. That can happen in a later PR (but ideally before merging #2856).

Comment thread public/index.css Outdated
margin-top: 0.5rem;
}

.content > section > :is(ul, ol) :has(kbd) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:has() isn’t supported in Firefox just yet, so this is only a partial fix and this approach also results in this mix of line heights which I think doesn’t look amazing:

image

I think I might suggest a multi-part solution. This PR on the left, my suggestion on the right:
image

How to do it:

  1. Bump up line-height everywhere to 1.75. This is a small change and I actually kind of like the extra breathing room it gives.

  2. Tweak kbd styles to take up slightly less vertical space:

    • box-shadow: 0 2px var(--theme-shade-subtle) — 2px instead of the current 3px

    • padding: 0.0625rem 0.375rem — 1px instead of 2px vertical padding

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I considered "everywhere" as in, content paragraphs & lists, which I liked, but bumping everywhere to 1.75 simply didn't feel right for some reason.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Haha, thanks for interpreting me correctly, this was exactly what I meant by “everywhere” 😅

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome — thanks for fixing this and the <Box> styles Yan! 💜

Comment on lines +125 to +132
:global([aria-selected='true']) .segment:not(.done) {
stroke: hsl(var(--color-gray-20));
}

:global(.theme-dark [aria-selected='true']) .segment:not(.done) {
stroke: hsl(var(--color-gray-70));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While it’s true that now you can see the empty segments better, doesn’t this now mean the contrast between complete/incomplete segments is too low?

If we remove colours it’s very subtle that these are different:

image

I’d suggest making the empty segments only much more slightly lighter than they are currently (or leaving as-is — I think arguably it’s the complete segments that are the key information and you can see the empty space in contrast to those).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Tweaked these a bit. I guess this works as a compromise, but I'm fine with leaving it as-is if you still think it isn't enough contrast. I reduced the saturation of the images to zero to try emulate what you did in your screenshot.

Dark:
image
image

Light:
image
image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is good!

I took my screenshot using Firefox’s “Simulate Achromatopsia” option in the Accessibility tab in dev tools, which is useful for testing this and other color vision variations:

image

Comment thread src/components/tutorial/TutorialNav.astro
@yanthomasdev
Copy link
Copy Markdown
Member Author

Thanks @delucis, made changes according to your suggestions. FYI, I can see the tutorial being properly tracked between translations, I probably thought it didn't because my local storage is constantly deleted.

Copy link
Copy Markdown
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.

LGTM! Thanks for taking care of this.

One thing still to think about in the future is if we need to handle RTL layout in the keyboard navigation of <TutorialNav>. Currently right arrow moves to the next tab, which is to the left in RTL. I’d need to research whether an RTL keyboard maps that already or if we need to flip that ourselves. Either way that’s non-blocking, and I don’t think we’re likely to get an Arabic tutorial translation anytime soon.

@yanthomasdev
Copy link
Copy Markdown
Member Author

I’d need to research whether an RTL keyboard maps that already or if we need to flip that ourselves. Either way that’s non-blocking, and I don’t think we’re likely to get an Arabic tutorial translation anytime soon.

That's something I haven't considered, good point! Just saw they do this manually in the Material UI repo and the Material Design guidelines kinda implies this has to be done manually.

@yanthomasdev yanthomasdev merged commit cfea387 into main Mar 16, 2023
@yanthomasdev yanthomasdev deleted the yan/tutorial-improvements branch March 16, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a11y Anything to do with improving accessibility site improvement Some thing that improves the website functionality - ask @delucis for help! tutorial Content/UI for the Build a Blog Tutorial

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants