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

docs: fix js and ts extension when TSToggle is active #9486

Merged
merged 9 commits into from
Apr 17, 2023

Conversation

vicentematus
Copy link
Contributor

@vicentematus vicentematus commented Mar 22, 2023

The Problem

Solves #9431. When switching to the TypeScript mode in the docs, the .js and .ts references didnt change to their specific language.

The only change happened on the file name of the code blocks (because its already implemented specifically for it).

Describe the solution

The way of dealing with typescript toggle is the following:

function toggle(checked) {
try {
document.documentElement.classList.toggle('prefers-ts', checked);
localStorage.setItem('prefers-ts', checked);
} catch (e) {
// localStorage not available or we are on the server
}
}
function prefers_ts() {
try {
return localStorage.getItem('prefers-ts') === 'true' ? true : false;
} catch (e) {
return false;
}
}

Which adds the following CSS classes to the DOM:

.ts-version {
display: none;
}
.prefers-ts .js-version {
display: none;
}
.prefers-ts .ts-version {
display: block;
}
.no-js .ts-toggle {
display: none;
}

To avoid duplicate files for every markdown documentation, is to modify the output of <code> and <heading> element whenever they have a .js or a .ts extension using a conditional regex.

If they contain that conditional regex, we delete the extension from the string and then render the code element with both available extensions as follows:

<code>+page <span class="js-version">.js</span> <span class="ts-version">.ts</span></code>

We modify the return statement to check if the conditional regex (.js or .ts extension) is true, then return the span element with both available extensions.

I added styleinline-flex to the <heading> element, because the css properties for .prefers-ts and .prefers-js are for block elements. In that way we can avoid overflow.

Improvements

Maybe there's better ways to handle the conditional statement and return the span element.

2023-03-22.16-53-01.mp4

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@changeset-bot
Copy link

changeset-bot bot commented Mar 22, 2023

⚠️ No Changeset found

Latest commit: 3ba20a9

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

@dummdidumm
Copy link
Member

@geoffrich accessibility-wise, is there anything speaking against this? For example screen readers who list both extension names.

@geoffrich
Copy link
Member

It shouldn't be an issue -- the non-active extension will be display: none and won't be read out by a screen reader.

Though I did notice this issue in the load docs. We should spot-check where we already mention something like "+page.js or +page.ts" and update it since the file extension will automatically change.

image

@vicentematus
Copy link
Contributor Author

@geoffrich Are you refering to update the markdown files where .js or .ts are mentioned simultaneously as example?

For example: the one you mentioned in the load docs.

Solution: i can check all the .md files that contains a .js extension and check the context if they mention .ts or .js at the same time. If true, then i would delete the 2nd example. Like the example you gave.

If you agree i can do this. @dummdidumm

@dummdidumm
Copy link
Member

Mhhhhm tough call. As a reader, if you didn't notice the toggle already, you might think "oh so SvelteKit only works with JavaScript?" - on the other hand, there are many places where only once is used. I'm inclined to say we should remove the double mentions. @Rich-Harris what's your stance on this?

@@ -371,7 +388,13 @@ function parse({ file, body, code, codespan }) {
throw new Error(`Unexpected <h${level}> in ${file}`);
}

return `<h${level} id="${slug}">${html}<a href="#${slug}" class="permalink"><span class="visually-hidden">permalink</span></a></h${level}>`;
return `<h${level} id="${slug}" style="display: inline-flex">${
Copy link
Member

Choose a reason for hiding this comment

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

we can't use inline-flex here — headings are supposed to look like this...

image

...but with this PR they look like this:

image

@Rich-Harris
Copy link
Member

I think removing the double mentions makes sense, but the toggle probably needs to be more discoverable somehow, whether that's by mentioning it in the docs where it makes sense to, or moving it somewhere else on the page, or something else. I don't have a great suggestion right now though.

Other than that, I love this change!

@vicentematus
Copy link
Contributor Author

vicentematus commented Mar 30, 2023

Great. So i will remove the double mentions on all the corresponding markdown files, where the string contains any .js and .ts mention at the same time.

but the toggle probably needs to be more discoverable somehow

I was watching how the Vue docs do it:

vuejs org_guide_introduction html

And how Deno Docs does it but for different versions.
deno land_manual@v1 32 1_getting_started_installation

It's a good idea to put in on the top-left side. In that way people before reading any section, they will always notice the Typescript or Javascript. Plus we follow the same pattern.

This is a fast sketch of how it woud look:

image

What do you think? @Rich-Harris @dummdidumm 👀

@dummdidumm
Copy link
Member

Let's worry about where to place the toggle later and just replace the double occurences with either/or for now 👍

@Rich-Harris Rich-Harris merged commit a243aa6 into sveltejs:master Apr 17, 2023
@Rich-Harris
Copy link
Member

thank you!

@vicentematus
Copy link
Contributor Author

thank you!

❤️

@vicentematus vicentematus deleted the docs/js-ts-extension branch August 3, 2023 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants