[WIP]create pages for examples, tutorials and videos, refactor examples #1192
Conversation
…to learning-center
josh-collinsworth
left a comment
josh-collinsworth
left a comment
There was a problem hiding this comment.
A lot of these comments are a bit nitpicky, but there are a few a11y issues I'd like to make sure we fix before launch.
Also: apologies if I jumped the gun on this review; I know it's still a draft.
_includes/raw_with_sidebar.tsx
Outdated
| > | ||
| </div> | ||
| <div | ||
| class="absolute top-0 bottom-0 left-0 right-0 xl:left-74 overflow-y-auto xl:grid xl:grid-cols-7 xl:gap-8 max-w-screen-2xl mx-auto" |
There was a problem hiding this comment.
Nit: inset-0 sets top, bottom, left, and right all together.
| return ( | ||
| <button | ||
| type="button" | ||
| aria-label="Copy code to clipboard" |
There was a problem hiding this comment.
Same note about buttons and .sr-only text here; better to use text than aria-label (and to put aria-hidden on the SVG, just to be safe).
| // import { join } from "@std/path"; | ||
| // import { walk } from "@std/fs"; | ||
| // import { assertEquals, assertNotMatch } from "@std/assert"; | ||
|
|
||
| // const decoder = new TextDecoder(); | ||
|
|
||
| // Deno.test("Check examples", async (t) => { | ||
| // for await (const item of walk("./learn/examples/")) { | ||
| // const path = join("examples", item.name); | ||
|
|
||
| // if (!path.endsWith(".ts")) continue; | ||
|
|
||
| // await t.step("Check graph: " + path, async () => { | ||
| // const result = await new Deno.Command(Deno.execPath(), { | ||
| // args: ["info", path], | ||
| // }).output(); | ||
| // assertEquals(result.code, 0); | ||
| // assertNotMatch(decoder.decode(result.stdout), /\(resolve error\)/); | ||
| // }); | ||
| // } | ||
| // }); |
There was a problem hiding this comment.
Should this just be deleted?
There was a problem hiding this comment.
no, it needs to be fixed so that the tests actually pass
| <label for="example" className="mr-4 ml-4 flex items-center"> | ||
| <ExampleIcon color="#9d9d9d" />Examples: | ||
| <span className="switch"> | ||
| <input | ||
| type="checkbox" | ||
| id="example" | ||
| value="Examples" | ||
| className="mr-2" | ||
| checked | ||
| /> | ||
| <span className="slider"></span> | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
Issue: there's no accessible text for these labels, and no visible focus indication.
Also: ideally, these toggles should be grouped somehow, probably by a <form> element.
There was a problem hiding this comment.
Is the accessible text not the text in the label?
| <p class="max-w-prose mx-auto text-center pt-4 mt-3"> | ||
| Need an example that isn't here? Or want to add one of your own?<br /> | ||
| {" "} | ||
| We welcome contributions!{" "} | ||
| <br />You can request more examples, or add your own at our{" "} | ||
| <a | ||
| href="https://github.com/denoland/deno-docs?tab=readme-ov-file#examples" | ||
| class="text-primary hover:underline focus:underline" | ||
| > | ||
| GitHub repository | ||
| </a> | ||
| </p> |
There was a problem hiding this comment.
It's best to avoid <br /> tags, as the user's font size, zoom, and/or screen size could make them appear in random, odd places.
Better solutions might be: using multiple paragraph tags; using text-balance; or setting a font-size-relative max-width.
learn_filter.client.ts
Outdated
| document.addEventListener("DOMContentLoaded", () => { | ||
| const tutorial = document.getElementById("tutorial")! as HTMLInputElement; | ||
| const example = document.getElementById("example")! as HTMLInputElement; | ||
| const video = document.getElementById("video")! as HTMLInputElement; | ||
| const listItems = document.getElementsByClassName("learning-list-item"); | ||
|
|
||
| const filterItems = () => { | ||
| const shown = [tutorial, example, video].filter((item) => item.checked); | ||
| const shownType = shown.map((item) => item.id); | ||
|
|
||
| for (const item of listItems) { | ||
| const htmlItem = item as HTMLElement; | ||
| const category = htmlItem.getAttribute("data-category"); | ||
| const enabled = shownType.includes(category!); | ||
| htmlItem.style.display = enabled ? "" : "none"; | ||
| } | ||
| }; | ||
|
|
||
| tutorial.addEventListener("change", () => filterItems()); | ||
| example.addEventListener("change", () => filterItems()); | ||
| video.addEventListener("change", () => filterItems()); | ||
|
|
||
| filterItems(); | ||
| }); |
There was a problem hiding this comment.
FWIW, since we're just using radio inputs, we could probably achieve this in CSS using the :has() selector.
There was a problem hiding this comment.
I'll take a look
There was a problem hiding this comment.
I've moved this over to CSS, but will require some assistance optimising it for tailwind
|
|
||
| .hub-header { | ||
| border-bottom: 1px solid var(--borderColor-muted, var(--color-border-muted)); | ||
| padding-bottom: 0.3em; | ||
| } | ||
|
|
||
| .switch { | ||
| position: relative; | ||
| width: 1.8rem; | ||
| height: 1rem; | ||
| display: block; | ||
| margin-left: 0.5rem; | ||
| margin-right: 1rem; | ||
| } | ||
|
|
||
| .switch input { | ||
| opacity: 0; | ||
| width: 0; | ||
| height: 0; | ||
| } | ||
|
|
||
| .slider { | ||
| position: absolute; | ||
| cursor: pointer; | ||
| top: 0; | ||
| left: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| background-color: #ccc; | ||
| -webkit-transition: 0.4s; | ||
| transition: 0.4s; | ||
| border-radius: 1rem; | ||
| } | ||
|
|
||
| .slider:before { | ||
| position: absolute; | ||
| content: ""; | ||
| height: calc(1rem - 4px); | ||
| width: calc(1rem - 4px); | ||
| left: 2px; | ||
| bottom: 2px; | ||
| background-color: white; | ||
| -webkit-transition: 0.4s; | ||
| transition: 0.4s; | ||
| border-radius: 50%; | ||
| } | ||
|
|
||
| input:checked + .slider { | ||
| background-color: #2196f3; | ||
| } | ||
|
|
||
| input:focus + .slider { | ||
| box-shadow: 0 0 1px #2196f3; | ||
| } | ||
|
|
||
| input:checked + .slider:before { | ||
| transform: translateX(calc(0.8rem)); | ||
| } |
There was a problem hiding this comment.
As a general rule, I think we should do everything we can in Tailwind, rather than maintaining two separate sources of truth for styling.
If we want to reuse styles (as with the .switch behavior, for example), then it's better to just create a reusable component with all the markup and classes, rather than have some styles in one place and some in another.
(I know this may be driven partially by dislike of Tailwind, which I can relate to if so, but I think it's better to stick with the system.)
There was a problem hiding this comment.
Yes, this was meant as a 'PoC' before John gets his hands on the design work. I wasn't sure if the toggle idea would be accepted so this was some throwaway until it is.
(#1191)