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

Some symbols in code blocks in the reference (the description, not the examples) are msitakenly HTML-escaped #447

Closed
davepagurek opened this issue Jul 7, 2024 · 5 comments · Fixed by #554
Labels
Bug Something isn't working

Comments

@davepagurek
Copy link
Collaborator

Most appropriate sections of the p5.js website?

Reference

What is your operating system?

Mac OS

Web browser and version

Firefox

Actual Behavior

On https://p5js.org/reference/p5/mousePressed/, it contains this code snippet:

if (mouseX < 50) {
  // Code to run if the mouse is on the left.
}

Expected Behavior

if (mouseX < 50) {
  // Code to run if the mouse is on the left.
}

Steps to reproduce

Go to https://p5js.org/reference/p5/mousePressed/

Would you like to work on the issue?

Need to discuss the fix first

@davepagurek davepagurek added the Bug Something isn't working label Jul 7, 2024
@Abhinavcode13
Copy link
Contributor

if (mouseX &lt; 50) {
  // Code to run if the mouse is on the left.
}

don't this work fine ?

@davepagurek
Copy link
Collaborator Author

When users write code in the p5 web editor, they should be writing <, not &lt;, so the &lt; should not end up visible to users in the docs. Behind the scenes for this website it may still need to be represented like that, but we should make sure it gets rendered as the HTML character it represents instead.

@Abhinavcode13
Copy link
Contributor

so the most feasible fix should be to replace the &lt with < ?

@davepagurek
Copy link
Collaborator Author

It starts as < in the source of the documentation here:

https://github.com/processing/p5.js/blob/db01247bd224b71fbf253a546f6a35e65b988267/src/events/mouse.js#L1101-L1104

It's also still a < after it gets imported into the website repo, so the import script isn't the problem:

if (mouseX < 50) {
// Code to run if the mouse is on the left.
}

It looks like we're intentionally escaping the contents of <code> blocks here:

export const escapeCodeTagsContent = (htmlString: string): string => {
// Load the HTML string into Cheerio
const $ = load(htmlString);
// Loop through all <code> tags
$("code").each(function () {
// Get the current text and HTML inside the <code> tag
const currentHtml = $(this).html() ?? "";
// Use he to escape HTML entities
const escapedHtml = he.escape(currentHtml);
// Update the <code> tag content with the escaped HTML
$(this).html(escapedHtml);
});
// Return the modified HTML as a string
return $.html();
};

Looking at the git history, this was to address #95, which is for inline code. I think what we want is to have this apply just to block code. So that would mean escaping <code> tag contents, but not the contents of <pre><code> I think? And we would need to do that in the escape function I referenced above.

@Qianqianye
Copy link
Collaborator

Qianqianye commented Sep 12, 2024

We noticed more issues about symbols and characters (like "<>") render incorrectly on the reference page, as mentioned in issue #522 #538, and PR #525 . Let's consolidate our discussion on how to fixes this in the issue.

Pasting comments from @limzykenneth in PR#525

For rendering in the page I'm not sure if Astro or the current build automatically encodes the characters so we need to check there first. For URL, adding a step to encode the URL path may be enough to solve this without needing to change the reference in the main p5.js repo.

Would love to hear the contributors involved in this to share their thoughts @limzykenneth @davepagurek @nickmcintyre @Abhinavcode13 @goldwasser @meganmckissack @isZhiyangWang. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants